On Fri, Sep 15, 2023 at 4:45 PM Andrew MacLeod <amacl...@redhat.com> wrote:
>
> Ive been looking at __builtin_unreachable () regressions. The
> fundamental problem seems to be a lack of consistent expectation for
> when we remove it earlier than the final pass of VRP. After looking
> through them, I think this provides a sensible approach.
>
> Ranger is pretty good at providing ranges in blocks dominated by the
> __builtin_unreachable branch, so removing it isn't quite a critical as
> it once was. Its also pretty good at identifying what in the block can
> be affected by the branch.
>
> This patch provide an alternate removal algorithm for earlier passes.
> it looks at *all* the exports from the block, and if the branch
> dominates every use of all the exports, AND none of those values access
> memory, VRP will remove the unreachable call, rewrite the branch, update
> all the values globally, and finally perform the simple DCE on the
> branch's ssa-name. This is kind of what it did before, but it wasn't
> as stringent on the requirements.
>
> The memory access check is required because there are a couple of test
> cases for PRE in which there is a series of instruction leading to an
> unreachable call, and none of those ssa names are ever used in the IL
> again. The whole chunk is dead, and we update globals, however
> pointlessly. However, one of ssa_names loads from memory, and a later
> passes commons this value with a later load, and then the unreachable
> call provides additional information about the later load. This is
> evident in tree-ssa/ssa-pre-34.c. The only way I see to avoid this
> situation is to not remove the unreachable if there is a load feeding it.
>
> What this does is a more sophisticated version of what DOM does in
> all_uses_feed_or_dominated_by_stmt. THe feeding instructions dont have
> to be single use, but they do have to be dominated by the branch or be
> single use within the branches block..
>
> If there are multiple uses in the same block as the branch, this does
> not remove the unreachable call. If we could be sure there are no
> intervening calls or side effects, it would be allowable, but this a
> more expensive checking operation. Ranger gets the ranges right anyway,
> so with more passes using ranger, Im not sure we'd see much benefit from
> the additional analysis. It could always be added later.
>
> This fixes at least 110249 and 110080 (and probably others). The only
> regression is 93917 for which I changed the testcase to adjust
> expectations:
>
> // PR 93917
> void f1(int n)
> {
> if(n<0)
> __builtin_unreachable();
> f3(n);
> }
>
> void f2(int*n)
> {
> if(*n<0)
> __builtin_unreachable();
> f3 (*n);
> }
>
> We were removing both unreachable calls in VRP1, but only updating the
> global values in the first case, meaning we lose information. With the
> change in semantics, we only update the global in the first case, but we
> leave the unreachable call in the second case now (due to the load from
> memory). Ranger still calculates the contextual range correctly as [0,
> +INF] in the second case, it just doesn't set the global value until
> VRP2 when it is removed.
>
> Does this seem reasonable?
I wonder how this addresses the fundamental issue we always faced
in that when we apply the range this range info in itself allows the
branch to the __builtin_unreachable () to be statically determined,
so when the first VRP pass sets the range the next pass evaluating
the condition will remove it (and the guarded __builtin_unreachable ()).
In principle there's nothing wrong with that if we don't lose the range
info during optimizations, but that unfortunately happens more often
than wanted and with the __builtin_unreachable () gone we've lost
the ability to re-compute them.
I think it's good to explicitly remove the branch at the point we want
rather than relying on the "next" visitor to pick up the global range.
As I read the patch we now remove __builtin_unreachable () explicitly
as soon as possible but don't really address the fundamental issue
in any way?
> Bootstraps on x86_64-pc-linux-gnu with no regressions. OK?
>
> Andrew
>
>
