Re: [PATCH] fold-const: Handle AND, IOR, XOR with stepped vectors [PR112971].

[Richard Sandiford <richard.sandiford@arm.com>]

Richard Biener <rguenther@suse.de> writes:
> On Tue, 19 Dec 2023, Andrew Pinski wrote:
>
>> On Tue, Dec 19, 2023 at 2:40?AM Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>> >
>> > Richard Biener <rguenther@suse.de> writes:
>> > > On Tue, 19 Dec 2023, juzhe.zhong@rivai.ai wrote:
>> > >
>> > >> Hi, Richard.
>> > >>
>> > >> After investigating the codes:
>> > >> /* Return true if EXPR is the integer constant zero or a complex constant
>> > >>    of zero, or a location wrapper for such a constant.  */
>> > >>
>> > >> bool
>> > >> integer_zerop (const_tree expr)
>> > >> {
>> > >>   STRIP_ANY_LOCATION_WRAPPER (expr);
>> > >>
>> > >>   switch (TREE_CODE (expr))
>> > >>     {
>> > >>     case INTEGER_CST:
>> > >>       return wi::to_wide (expr) == 0;
>> > >>     case COMPLEX_CST:
>> > >>       return (integer_zerop (TREE_REALPART (expr))
>> > >>               && integer_zerop (TREE_IMAGPART (expr)));
>> > >>     case VECTOR_CST:
>> > >>       return (VECTOR_CST_NPATTERNS (expr) == 1
>> > >>               && VECTOR_CST_DUPLICATE_P (expr)
>> > >>               && integer_zerop (VECTOR_CST_ENCODED_ELT (expr, 0)));
>> > >>     default:
>> > >>       return false;
>> > >>     }
>> > >> }
>> > >>
>> > >> I wonder whether we can simplify the codes as follows :?
>> > >>       if (integer_zerop (arg1) || integer_zerop (arg2))
>> > >>         step_ok_p = (code == BIT_AND_EXPR || code == BIT_IOR_EXPR
>> > >>                      || code == BIT_XOR_EXPR);
>> > >
>> > > Possibly.  I'll let Richard S. comment on the whole structure.
>> >
>> > The current code is handling cases that require elementwise arithmetic.
>> > ISTM that what we're really doing here is identifying cases where
>> > whole-vector arithmetic is possible instead.  I think that should be
>> > a separate pre-step, rather than integrated into the current code.
>> >
>> > Largely this would consist of writing out match.pd-style folds in
>> > C++ code, so Andrew's fix in comment 7 seems neater to me.
>> 
>> I didn't like the change to match.pd (even with a comment on why)
>> because it violates the whole idea behind canonicalization of
>> constants being 2nd operand of commutative and comparison expressions.
>> Maybe there are only a few limited match/simplify patterns which need
>> to add the :c for constants not being the 2nd operand but there needs
>> to be a comment on why :c is needed for this.
>
> Agreed.  Note that in theory we of course could define extra
> canonicalization rules for CST op CST in tree_swap_operands_p,
> there are some cases those surivive, mostly around FP and
> dynamic rounding state.  I'd rather go that route if we decide
> that match.pd should catch this.

I don't think that'll help in all cases though.  E.g. consider an
unfoldable:

  (or 1 CST)

where CST is "complicated".  We'd probably canonicalise that to:

  (or CST 1)

And that's good if we have:

  (or (or CST 1) 2)

since it could be folded to:

  (or CST 3)

But there are other constants CST2 for which (or CST CST2) is foldable
and (op 1 CST2) isn't.  So:

  (or (or 1 CST) CST2)

would sometimes be foldable in cases where:

  (or (or CST 1) CST2)

isn't.

>> >
>> > But if this must happen in const_binop instead, then we could have
>> > a function like:
>> 
>> The reasoning of why it should be in const_binop rather than in match
>> is because both operands are constants.
>
> +1

I can see that argument for the traditional case where all constants
can be folded at compile time.  But that isn't the case for VLA constants.
(op CST1 CST2) is sometimes not knowable at compile time.  And I think
match.pd should apply to those cases just as much as to (op VAR CST).

VLA vector "constants" are really in intermediate category between
variable and constant.

The approach that the patch takes is to add a "rule of thumb" that
applies to all (op X CST), regardless of whether X is constant.
It doesn't work by doing constant arithmetic per se.  If we add
those rules of thumb here, I think it'll keep growing and growing.

Thanks,
Richard