On 6/5/2026 11:12 AM, 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.
I think that's the key idea I missed when reviewing the PR. It's less
about what unswitching did and more about the fundamental nature of the
relationship and global ranges. So with that cleared up. OK for the
trunk.
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 :-)
No doubt.
Thanks!
jeff
