| Message ID | 87bmm51s2o.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | match.pd handling of three-constant bitops | expand |
On Wed, Sep 20, 2017 at 2:18 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > natch.pd tries to reassociate two bit operations if both of them have > constant operands. However, with the polynomial integers added later, > there's no guarantee that a bit operation on two integers can be folded > at compile time. This means that the pattern can trigger for operations > on three constants, and as things stood could endlessly oscillate > between the two associations. > > This patch keeps the existing pattern for the normal case of a > non-constant first operand. When all three operands are constant it > tries to find a pair of constants that do fold. If none do, it keeps > the original expression as-was. > > Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linus-gnu. > OK to install? Hmm, is the complication with trying variants necessary or could we simply do if (!CONSTANT_CLASS_P (@0)) (bitop @0 (bitop @1 @2)) and be done with it? > Richard > > > 2017-09-20 Richard Sandiford <richard.sandiford@linaro.org> > Alan Hayward <alan.hayward@arm.com> > David Sherwood <david.sherwood@arm.com> > > gcc/ > * match.pd: Handle bit operations involving three constants > and try to fold one pair. > > Index: gcc/match.pd > =================================================================== > --- gcc/match.pd 2017-09-16 21:38:21.106513157 +0100 > +++ gcc/match.pd 2017-09-20 13:17:10.552389270 +0100 > @@ -1017,7 +1017,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > (for bitop (bit_and bit_ior bit_xor) > (simplify > (bitop (bitop @0 CONSTANT_CLASS_P@1) CONSTANT_CLASS_P@2) > - (bitop @0 (bitop @1 @2)))) > + (if (!CONSTANT_CLASS_P (@0)) > + (bitop @0 (bitop @1 @2)) > + (with { tree cst1 = const_binop (bitop, type, @0, @2); } > + (if (cst1) > + (bitop @1 { cst1; }) > + (with { tree cst2 = const_binop (bitop, type, @1, @2); } > + (if (cst2) > + (bitop @0 { cst2; })))))))) > > /* Try simple folding for X op !X, and X op X with the help > of the truth_valued_p and logical_inverted_value predicates. */
Richard Biener <richard.guenther@gmail.com> writes: > On Wed, Sep 20, 2017 at 2:18 PM, Richard Sandiford > <richard.sandiford@linaro.org> wrote: >> natch.pd tries to reassociate two bit operations if both of them have >> constant operands. However, with the polynomial integers added later, >> there's no guarantee that a bit operation on two integers can be folded >> at compile time. This means that the pattern can trigger for operations >> on three constants, and as things stood could endlessly oscillate >> between the two associations. >> >> This patch keeps the existing pattern for the normal case of a >> non-constant first operand. When all three operands are constant it >> tries to find a pair of constants that do fold. If none do, it keeps >> the original expression as-was. >> >> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linus-gnu. >> OK to install? > > Hmm, is the complication with trying variants necessary or could we simply do > > if (!CONSTANT_CLASS_P (@0)) > (bitop @0 (bitop @1 @2)) > > and be done with it? That's enough to fix the oscillation, but there are cases that benefit from trying the other combinations. E.g. if @1 and @2 are both INTEGER_CSTs, it's still worth folding them. POLY_CST | INTEGER_CST is supported for some values, so if @0 | @1 doesn't fold, it's worth trying @0 | @2 instead. Thanks, Richard
On Thu, Sep 21, 2017 at 1:17 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Wed, Sep 20, 2017 at 2:18 PM, Richard Sandiford >> <richard.sandiford@linaro.org> wrote: >>> natch.pd tries to reassociate two bit operations if both of them have >>> constant operands. However, with the polynomial integers added later, >>> there's no guarantee that a bit operation on two integers can be folded >>> at compile time. This means that the pattern can trigger for operations >>> on three constants, and as things stood could endlessly oscillate >>> between the two associations. >>> >>> This patch keeps the existing pattern for the normal case of a >>> non-constant first operand. When all three operands are constant it >>> tries to find a pair of constants that do fold. If none do, it keeps >>> the original expression as-was. >>> >>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linus-gnu. >>> OK to install? >> >> Hmm, is the complication with trying variants necessary or could we simply do >> >> if (!CONSTANT_CLASS_P (@0)) >> (bitop @0 (bitop @1 @2)) >> >> and be done with it? > > That's enough to fix the oscillation, but there are cases that benefit > from trying the other combinations. E.g. if @1 and @2 are both > INTEGER_CSTs, it's still worth folding them. POLY_CST | INTEGER_CST > is supported for some values, so if @0 | @1 doesn't fold, it's worth > trying @0 | @2 instead. Hmm, ok. The patch is ok then if you add a comment reflecting the above. Thanks, Richard. > > Thanks, > Richard
Richard Biener <richard.guenther@gmail.com> writes: > On Thu, Sep 21, 2017 at 1:17 PM, Richard Sandiford > <richard.sandiford@linaro.org> wrote: >> Richard Biener <richard.guenther@gmail.com> writes: >>> On Wed, Sep 20, 2017 at 2:18 PM, Richard Sandiford >>> <richard.sandiford@linaro.org> wrote: >>>> natch.pd tries to reassociate two bit operations if both of them have >>>> constant operands. However, with the polynomial integers added later, >>>> there's no guarantee that a bit operation on two integers can be folded >>>> at compile time. This means that the pattern can trigger for operations >>>> on three constants, and as things stood could endlessly oscillate >>>> between the two associations. >>>> >>>> This patch keeps the existing pattern for the normal case of a >>>> non-constant first operand. When all three operands are constant it >>>> tries to find a pair of constants that do fold. If none do, it keeps >>>> the original expression as-was. >>>> >>>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linus-gnu. >>>> OK to install? >>> >>> Hmm, is the complication with trying variants necessary or could we simply do >>> >>> if (!CONSTANT_CLASS_P (@0)) >>> (bitop @0 (bitop @1 @2)) >>> >>> and be done with it? >> >> That's enough to fix the oscillation, but there are cases that benefit >> from trying the other combinations. E.g. if @1 and @2 are both >> INTEGER_CSTs, it's still worth folding them. POLY_CST | INTEGER_CST >> is supported for some values, so if @0 | @1 doesn't fold, it's worth >> trying @0 | @2 instead. > > Hmm, ok. > > The patch is ok then if you add a comment reflecting the above. Thanks. For the record, here's what I installed after retesting. Richard 2018-01-03 Richard Sandiford <richard.sandiford@linaro.org> Alan Hayward <alan.hayward@arm.com> David Sherwood <david.sherwood@arm.com> gcc/ * match.pd: Handle bit operations involving three constants and try to fold one pair. ------------------------------------------------------------------------------ Index: gcc/match.pd =================================================================== --- gcc/match.pd 2018-01-02 16:55:03.498204314 +0000 +++ gcc/match.pd 2018-01-03 07:10:06.993953631 +0000 @@ -1111,7 +1111,24 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (for bitop (bit_and bit_ior bit_xor) (simplify (bitop (bitop @0 CONSTANT_CLASS_P@1) CONSTANT_CLASS_P@2) - (bitop @0 (bitop @1 @2)))) + (if (!CONSTANT_CLASS_P (@0)) + /* This is the canonical form regardless of whether (bitop @1 @2) can be + folded to a constant. */ + (bitop @0 (bitop @1 @2)) + /* In this case we have three constants and (bitop @0 @1) doesn't fold + to a constant. This can happen if @0 or @1 is a POLY_INT_CST and if + the values involved are such that the operation can't be decided at + compile time. Try folding one of @0 or @1 with @2 to see whether + that combination can be decided at compile time. + + Keep the existing form if both folds fail, to avoid endless + oscillation. */ + (with { tree cst1 = const_binop (bitop, type, @0, @2); } + (if (cst1) + (bitop @1 { cst1; }) + (with { tree cst2 = const_binop (bitop, type, @1, @2); } + (if (cst2) + (bitop @0 { cst2; })))))))) /* Try simple folding for X op !X, and X op X with the help of the truth_valued_p and logical_inverted_value predicates. */
Index: gcc/match.pd =================================================================== --- gcc/match.pd 2017-09-16 21:38:21.106513157 +0100 +++ gcc/match.pd 2017-09-20 13:17:10.552389270 +0100 @@ -1017,7 +1017,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (for bitop (bit_and bit_ior bit_xor) (simplify (bitop (bitop @0 CONSTANT_CLASS_P@1) CONSTANT_CLASS_P@2) - (bitop @0 (bitop @1 @2)))) + (if (!CONSTANT_CLASS_P (@0)) + (bitop @0 (bitop @1 @2)) + (with { tree cst1 = const_binop (bitop, type, @0, @2); } + (if (cst1) + (bitop @1 { cst1; }) + (with { tree cst2 = const_binop (bitop, type, @1, @2); } + (if (cst2) + (bitop @0 { cst2; })))))))) /* Try simple folding for X op !X, and X op X with the help of the truth_valued_p and logical_inverted_value predicates. */