On 9/18/26 12:18 AM, Richard Biener wrote:
On Wed, Sep 16, 2026 at 10:55 PM Yongqiang Tian <[email protected]> wrote:
Thanks for pointing this out. I had not considered the debug-value
lifetime consequence when I sent the patch.
The reason for the proposed change is that the DEBUG_INSN in the
reproducer mentions if_info->x before the jump. Treating that debug-only
reference as an ordinary use makes noce_get_alt_condition reject the
min/max conversion with debug information, while the same ordinary RTL
is converted without debug information.
However, simply ignoring that reference means that the generated min
instruction can subsequently overwrite the same pseudo. If the debug
location were allowed to continue referring to that register after the
overwrite, it would indeed describe the wrong value.
I traced the testcase through ce2 and vartrack using the RTL dumps. After
if-conversion, the relevant ordering is:
(var_location xghost (reg ... a))
(set (reg ... a) (smin ...))
Thus, the original debug bind is still valid before the new smin. For
the submitted testcase, vartrack then preserves the old value after the
smin by changing the location to an entry_value expression.
I also tested a stronger variant in which the incoming value of a is
first changed by an inline asm, so that the value held by xghost cannot
be recovered from the function entry value. In that case, vartrack emits:
(var_location xghost (nil))
after the smin, rather than extending the overwritten register location.
So, for these two cases, the later var-tracking pass handles both
outcomes: it recovers the old value when possible and marks the location
unavailable when it is not. The debug bind before the smin remains valid
up to the point where the register is overwritten.
For completeness, I also tested the exact submitted patch, commit
16893fa6ec3e10d4bfe605e3bfc8afe85fde8aa2, against the unmodified
baseline at f311d59c4d45f5042029647c88966e2e48330adf.
For the native testing, build, host, and target were all
x86_64-pc-linux-gnu. I configured both trees with:
configure --disable-multilib
and ran:
make -j8 bootstrap
make -j8 -k check
The bootstrap and complete testsuite run finished successfully. Comparing
the generated .sum files against the baseline showed no new FAIL, XPASS,
ERROR, or UNRESOLVED results. The seven pre-existing unexpected outcomes
were identical between the baseline and patched trees.
I also built a C cross compiler with build and host
x86_64-pc-linux-gnu and target riscv64-unknown-linux-gnu. The
gcc.target/riscv/pr127423.c regression test passed in all seven testsuite
variations: -O0, -O1, -O2, -O3 -g, -Os, -Og -g, and -Oz.
That said, I am not sufficiently familiar with the intended RTL debug
maintenance rules to know whether relying on vartrack here is the right
design, or whether ifcvt is expected to reset or propagate the affected
debug uses explicitly when committing the transformation. Would you
prefer the latter? I would be happy to revise the patch accordingly.
What matters is the RTL IL semantics after the ifcvt transform, not so
much what the vartrack machinery in the end makes of it (that matters
in practice, of course). So if after ifcvt the debug-insn refers to a register
that is not set or set to the wrong value the debug-insn should be at least
reset. I can't tell from the above whether the IL is correct or not and whether
this is by design or accidential with the testcase.
This patch merely makes firing of the transformation independent of
whether or not there are DEBUG_INSNs in the IL at inconvenient places.
That's inherently the right thing to do and similar to other places that
must ignore DEBUG_INSNs when making code generation decisions.
If there are concerns about DEBUG_INSN maintenance, those are
independent of this patch. Similarly if there are concerns about the IL
correctness of the ifcvt transformation, those are also independent of
this patch.
Jeff
Richard.
