On 6/5/26 11:35, Jeffrey Law wrote:
On 6/5/2026 8:51 AM, Andrew MacLeod wrote:
If the condition leading to a __builtin_unreachable involves more
than one SSA name, it is unsafe to assign a global range to an SSA
name even if all current uses are valid
if (a == 0)
return
<...>
if (a == b)
= b
else
__builtin_unreachable ()
DOM thinks all uses of b can be given the global value of [1, +INF]
because a value of 0 will hit the __builtin_unreachable() call.
It does not take into account that b has a relation with a, and if a
later pass moves these conditions around, that assumption may not be
valid any longer.
VRP refuses to attempt early resolution of builtin_unreachable ()
calls if the expression leading the builtin_unreachable() contains 2
SSA names as the relation introduced makes it unsafe.
This patch give DOM the same early exit... check if there are 2 SSA
names on the condition and not try to assign a global range if so.
Bootstraps on x86_64-pc-linux-gnu and no regressions. (Presumably..
there appear to be spurious avx testcases failing that seem unrelated
to whatever patch I apply)
My big question is do we have a deeper problem here. It sounds like
unswitch swapped two statements which invalidated a global range that
DOM had recorded? Doesn't that really point to a problem with
unswitch in that the unswitch transformation invalidated a global
range without clearing it? Are there any other places where we could
record a global range based on properties that unswitch (or another
pass) might change?
There's a long discussion in the PR. I cannot comment on the safety of
what unswitching does in general, but I can comment on the safety of
assigning the global.
When I rewrote the __builtin_unreachable (), I found it was generally
unsafe to export global values early unless *All* values generated by
the edge could be replaced with the value generated by the edge.
So its safe for
if (a == 0) __builtin_unreachable()
to set the global range for a to [1, +INF] if this dominates all uses of a.
Given
if (a == b) __builtin_unreachable ()
Its only safe to to set global ranges if both a *and* b can be set...
ie, its unsafe to set 'b' to some range, and not set 'a'. There is a
relation introduced between them, and if the global value do not reflect
the relation, we can get into trouble later.
In this testcase, we can not propagate both values to be [1, +INF], and
that equality introduced ended up causing the issue when some code was
moved around.
The need to assign global values early like this is far less important
than it once was.. That same information is available from ranger
everywhere, and globals should really only be important once we are set
to drop from SSA. EVRP and VRP1 will do some early glpobal assignments,
but only proven safe ones. VRP2 drops the builtin assumes and does what
it can for globals safely at that point. I don't think we should be
applying contextual derived information to global values before then
unless we are 100% ultra sure its safe.
My 2 cents.
Andrew
PS, and clearing global information is not a good idea if at all
avoidable.. that information may have come from IPA or elsewhere and
not be reproducible. As long as we never lie about a global range, we
shouldn't have any problems moving code around :-)
