gh-109889: fix compiler's redundant NOP detection to look past NOPs with no lineno when looking for the next instruction's lineno

[iritkatriel]

Fixes #109889.

• Issue: flowgraph optimize_cfg Assertion for if-else-expression with if-else-expression as condition #109889

[carljm]

Initially, I thought it would be simpler to have the same behavior whether the next instruction is in the same basicblock or the next one; i.e. it would be clearer to just match the behavior above on lines 1004-1011. If the next instruction has no location, set it to this NOP's location and remove this NOP.

But then I realized that this is not actually equivalent in the case where we are crossing to the next basicblock, because the next basicblock may be a jump target, and that jump should not report the location from the current NOP. So we can't safely use the "copy location to next instr without location" approach across the basicblock boundary.

I'm not sure if this subtlety will be obvious to future readers/modifiers of this code?

We could safely use this new behavior (skip NOP without lineno that will be removed) in all cases, and restructure the code so that first we find the next "relevant" instruction (whether in current or next basic block), and then compare its location with the current NOP. I think this would be somewhat clearer and simpler, but it's up to you whether you agree / think it's worth it.

[iritkatriel]

We could safely use this new behavior (skip NOP without lineno that will be removed) in all cases

I think we are a bit more aggressive when it's within the same basic block: we will hand the location on to any instruction that doesn't have a line number, not just a NOP.

[markshannon]

If I remove the assert statement on main and compile the test function, I see no NOPs:

Disassembly of <code object f at 0x7f1a9a409800, file "<stdin>", line 3>:
  3           0 RESUME                   0

  4           2 LOAD_GLOBAL              0 (b)
             12 TO_BOOL
             20 POP_JUMP_IF_FALSE        7 (to 38)
             24 LOAD_GLOBAL              4 (a)
             34 POP_TOP
             36 RETURN_CONST             0 (None)
        >>   38 LOAD_GLOBAL              2 (c)
             48 TO_BOOL
             56 POP_JUMP_IF_FALSE        7 (to 74)
             60 LOAD_GLOBAL              4 (a)
             70 POP_TOP
             72 RETURN_CONST             0 (None)
        >>   74 LOAD_GLOBAL              6 (d)
             84 POP_TOP
             86 RETURN_CONST             0 (None)

Is the change to the optimizer necessary, is there a case where it generates better output?
If not, then maybe just remove the assert?

[iritkatriel]

If that's the case then we should try to understand where the NOP gets removed. I wouldn't remove the assertion before I understand what's going on.

[miss-islington]

Thanks @iritkatriel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-app]

GH-110048 is a backport of this pull request to the 3.12 branch.

[iritkatriel]

I don't know why we do this in the optimizer and not in the assembler, after line numbers are propagated so it's easier to see what's redundant and what's not.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Debian root 3.x has failed when building commit f580edc.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/345/builds/5951) and take a look at the build logs.
4. Check if the failure is related to this commit (f580edc) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/345/builds/5951

Failed tests:

• test_signal

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/root/buildarea/3.x.angelico-debian-amd64/build/Lib/test/test_signal.py", line 1287, in test_stress_delivery_dependent
    self.assertEqual(len(sigs), N, "Some signals were lost")
AssertionError: 4726 != 10000 : Some signals were lost