THis PR is open against GC 14,15, and 16 as well. Should I apply it to
all those branches, or selectively which ones?
Should be a conservatively harmless fix...
Andrew
On 6/5/26 13:12, Andrew MacLeod wrote:
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 :-)
