On Mon, 7 Sep 2026, Andrea Pinski wrote: > On Mon, Sep 7, 2026 at 8:18 PM Andrea Pinski > <[email protected]> wrote: > > > > On Mon, Sep 7, 2026 at 12:30 PM Dominic P <[email protected]> wrote: > > > > > > Follow-up to r17-3550, extending movement_possibility_1 to variable shift > > > counts as suggested in > > > https://gcc.gnu.org/pipermail/gcc-patches/2026-August/728576.html > > > > > > Querying ranger turns out not to be safe here. A contextual range need > > > not > > > hold at the location the statement moves to, and a global range can be > > > derived from a __builtin_unreachable guard that has since been removed > > > after > > > bounding only the name's uses (tree-vrp.cc, remove_unreachable): an > > > earlier > > > version of this patch used the global range and hoisted a shift above > > > such a > > > guard, miscompiling function k in the new test. Instead accept a count > > > whose defining statement is a BIT_AND_EXPR with a constant mask below the > > > precision of the shifted type. This covers s &= 7 directly, and s % 8 > > > where > > > earlier passes rewrite it into that form. > > > > What about using the range on the incoming edge into the loop header? > > That is using range_on_edge with loop_preheader_edge? > > That should give you the range at the point. Maybe falling back to > > this code as a fallout. > > Actually when rewriting the shift that was hoisted, introduce a > BIT_AND_EXPR instead? This way you can hoist the shift no matter what > and you will have defined code still? if the value of the shift is > not used then there will be no issues; if the value is used it will be > the same as before. This might require more changes than your current > check but I think it might be more beneficial and you can hoist the > one for k still.
Adding a BIT_AND_EXPR is a possibility - I think we are using rewrite_to_defined_unconditional, but that does not handle shifts (yet), should it? Also there's the question of costs - LIM does some very very simplistic costing, should we see to avoid hoisting a shift if it is the only stmt in the invariant expression and it requires a BIT_AND? > > > > Thanks, > > Andrea > > > > > > > > Bootstrapped on x86_64-pc-linux-gnu; regression tested there with > > > gcc.dg/dg.exp and gcc.dg/tree-ssa/tree-ssa.exp, with no unexpected > > > results. > > > > > > Assisted-by: Claude Opus 5 (Anthropic) > > > > > > gcc/ChangeLog: > > > > > > * tree-ssa-loop-im.cc (movement_possibility_1): Allow a shift > > > or rotate by an SSA name whose defining statement masks the > > > count below the precision of the shifted type. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/tree-ssa/loop-im-shift-2.c: New test. > > > > > > Signed-off-by: Dominic P <[email protected]> > > > --- > > > .../gcc.dg/tree-ssa/loop-im-shift-2.c | 47 +++++++++++++++++++ > > > gcc/tree-ssa-loop-im.cc | 35 ++++++++++---- > > > 2 files changed, 73 insertions(+), 9 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/loop-im-shift-2.c > > > > > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/loop-im-shift-2.c > > > b/gcc/testsuite/gcc.dg/tree-ssa/loop-im-shift-2.c > > > new file mode 100644 > > > index 00000000000..6555ef92ef4 > > > --- /dev/null > > > +++ b/gcc/testsuite/gcc.dg/tree-ssa/loop-im-shift-2.c > > > @@ -0,0 +1,47 @@ > > > +/* { dg-do compile } */ > > > +/* { dg-options "-O2 -fdump-tree-lim2-details" } */ > > > + > > > +/* The count is masked into range, so the whole chain may be hoisted. */ > > > + > > > +void f (int *p, int n, int a, unsigned int s, int c, int *q) > > > +{ > > > + s &= 7; > > > + for (int i = 0; i < n; i++) > > > + if (p[i] > c) > > > + q[i] = ((a << s) * 5) / 7; > > > +} > > > + > > > +/* An unconstrained count keeps the chain in the loop. */ > > > + > > > +void g (int *p, int n, int a, unsigned int s, int c, int *q) > > > +{ > > > + for (int i = 0; i < n; i++) > > > + if (p[i] > c) > > > + q[i] = ((a << s) * 5) / 7; > > > +} > > > + > > > +/* A mask that does not bound the count below the precision. */ > > > + > > > +void h (int *p, int n, int a, unsigned int s, int c, int *q) > > > +{ > > > + s &= 63; > > > + for (int i = 0; i < n; i++) > > > + if (p[i] > c) > > > + q[i] = ((a << s) * 5) / 7; > > > +} > > > + > > > +/* The global range of s comes from a guard the hoist would bypass. */ > > > + > > > +void k (int *p, int n, int a, unsigned int s, int c, int *q) > > > +{ > > > + for (int i = 0; i < n; i++) > > > + if (p[i] > c) > > > + { > > > + if (s > 7) __builtin_unreachable (); > > > + q[i] = ((a << s) * 5) / 7; > > > + } > > > +} > > > + > > > +/* Only f's chain moves: the shift, the multiply and the divide. */ > > > +/* { dg-final { scan-tree-dump-times "Moving statement" 3 "lim2" } } */ > > > +/* { dg-final { scan-tree-dump-times "Moving statement \[^\\r\\n\]*<<" 1 > > > "lim2" } } */ > > > diff --git a/gcc/tree-ssa-loop-im.cc b/gcc/tree-ssa-loop-im.cc > > > index 0dcf5982ec2..a77241c8253 100644 > > > --- a/gcc/tree-ssa-loop-im.cc > > > +++ b/gcc/tree-ssa-loop-im.cc > > > @@ -419,15 +419,32 @@ movement_possibility_1 (gimple *stmt) > > > /* For shifts and rotates and possibly out-of-bound shift operands > > > we currently cannot rewrite them into something unconditionally > > > well-defined. */ > > > - if ((code == LSHIFT_EXPR > > > - || code == RSHIFT_EXPR > > > - || code == LROTATE_EXPR > > > - || code == RROTATE_EXPR) > > > - && (TREE_CODE (gimple_assign_rhs2 (stmt)) != INTEGER_CST > > > - /* We cannot use ranges at 'stmt' here. */ > > > - || wi::geu_p (wi::to_wide (gimple_assign_rhs2 (stmt)), > > > - element_precision (type)))) > > > - ret = MOVE_PRESERVE_EXECUTION; > > > + if (code == LSHIFT_EXPR > > > + || code == RSHIFT_EXPR > > > + || code == LROTATE_EXPR > > > + || code == RROTATE_EXPR) > > > + { > > > + tree count = gimple_assign_rhs2 (stmt); > > > + bool in_range = false; > > > + if (TREE_CODE (count) == INTEGER_CST) > > > + in_range = wi::ltu_p (wi::to_wide (count), > > > + element_precision (type)); > > > + else if (TREE_CODE (count) == SSA_NAME) > > > + { > > > + /* We cannot use ranges at 'stmt' here, contextual or > > > global; > > > + only a bound implied by the definition of the count holds > > > + where the statement is moved to. */ > > > + gimple *def = SSA_NAME_DEF_STMT (count); > > > + if (is_gimple_assign (def) > > > + && gimple_assign_rhs_code (def) == BIT_AND_EXPR > > > + && TREE_CODE (gimple_assign_rhs2 (def)) == INTEGER_CST > > > + && wi::ltu_p (wi::to_wide (gimple_assign_rhs2 (def)), > > > + element_precision (type))) > > > + in_range = true; > > > + } > > > + if (!in_range) > > > + ret = MOVE_PRESERVE_EXECUTION; > > > + } > > > } > > > > > > /* Non local loads in a transaction cannot be hoisted out. Well, > > > -- > > > 2.55.0 > > > > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Abhinav Puri; (HRB 36809, AG Nuernberg)
