On Mon, 21 Oct 2024, Tamar Christina wrote:
> > -----Original Message-----
> > From: Richard Biener <rguent...@suse.de>
> > Sent: Monday, October 21, 2024 9:55 AM
> > To: Tamar Christina <tamar.christ...@arm.com>
> > Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>
> > Subject: Re: [PATCH]middle-end: Handle more gcond lowering [PR117176]
> >
> > On Mon, 21 Oct 2024, Tamar Christina wrote:
> >
> > > Hi All,
> > >
> > > For boolean mask handling we have to lower BIT_NOT_EXPR for correctness
> > > into
> > > BIT_XOR_EXPR. Normally this is done through vect_recog_bool_pattern by
> > > following the defs of the gimple_assign.
> > >
> > > In the PR we ICE because early exits have the comparison inside the gcond
> > > itself and so vect_recog_bool_pattern does not get a chance to lower it.
> > >
> > > This patch changes it so vect_recog_gcond_pattern also does the lowering.
> > >
> > > The reason we ICE with SLP but not non-SLP is because for non-SLP the
> > > vectorization is driven from the gcond, where vectorizable_comparison_1
> > > rejects the operation. For SLP the body is vectorized before the root.
> > >
> > > My initial attempt was to try to re-use vect_recog_bool_pattern but this
> > > didn't
> > > work for the same reasons we didn't change vect_recog_bool_pattern to
> > > handle
> > > gconds.
> > >
> > > I also tried introducing an explicit assign for the conditional and having
> > > vect_recog_bool_pattern see the conditional. However this fails because
> > > vect_recog_bool_pattern and helpers only handle COND_EXPR, converts and
> > > operands that are being loaded from memory.
> > >
> > > Since I believe BIT_NOT_EXPR is the only one needing to be lowered for
> > > correctness, I just manually do so like we do in other places.
> >
> > So how does it work for BIT_IOR_EXPR/BIT_AND_EXPR and their feeding
> > stmts? How do those get transformed for the mask use? I'd have expected
> > the same mechanism to handle the BIT_NOT_EXPR case.
>
> It's only BIT_NOT that requires lowering because vectorizable_operation does
> not
> allow operations on bit precision types without any truncations being
> explicit.
>
> As such we only allow BIT_AND, BIT_XOR and BIT_IOR unmodified. Bit NOT needs
> to make the precision explicit by using the XOR.
>
> BIT_IOR_EXPR and BIT_AND_EXPR do not require mask handling transformations.
> e.g. vect_recog_mask_conversion_pattern explicitly doesn't transform them.
>
> And the only handling done for them in adjust_bool_pattern is a fixup for
> optimization reasons around combining with COND_EXPR here the arguments
> could have different precisions. i.e. X ^ (a > b ? 1 : 0).
But we also track mask uses vs. data uses, and ultimatively a mask
use results in the leafs to be converted to (signed-boolean:N). That's
also necessary for BIT_IOR_EXPR leading to a mask use and not different
from BIT_NOT_EXPRs. The path that does this should also handle the
BIT_NOT to BIT_XOR transform? See adjust_bool_pattern. So my point is,
when you need to handle BIT_NOT_EXPR in gcond lowering why don't you
need to also handle BIT_IOR_EXPR which in the end could be
an IOR of a BIT_NOT and something else?
Richard.
> Does this answer your question?
>
> Thanks,
> Tamar
> >
> > Richard.
> >
> > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > >
> > > Ok for master?
> > >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > PR tree-optimization/117176
> > > * tree-vect-patterns.cc (vect_recog_gcond_pattern): Lower
> > BIT_NOT_EXPR.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > > PR tree-optimization/117176
> > > * gcc.dg/vect/vect-early-break_130-pr117176.c: New test.
> > >
> > > ---
> > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_130-pr117176.c
> > b/gcc/testsuite/gcc.dg/vect/vect-early-break_130-pr117176.c
> > > new file mode 100644
> > > index
> > 0000000000000000000000000000000000000000..841dcce284dd7cff0c4f064
> > 8e6dc57082c8756c1
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_130-pr117176.c
> > > @@ -0,0 +1,21 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-add-options vect_early_break } */
> > > +/* { dg-require-effective-target vect_early_break } */
> > > +/* { dg-require-effective-target vect_int } */
> > > +
> > > +struct ColorSpace {
> > > + int componentCt;
> > > +};
> > > +
> > > +struct Psnr {
> > > + double psnr[3];
> > > +};
> > > +
> > > +int f(struct Psnr psnr, struct ColorSpace colorSpace) {
> > > + int i, hitsTarget = 1;
> > > +
> > > + for (i = 1; i < colorSpace.componentCt && hitsTarget; ++i)
> > > + hitsTarget = !(psnr.psnr[i] < 1);
> > > +
> > > + return hitsTarget;
> > > +}
> > > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> > > index
> > 746f100a0842090953571b535ff05375f46033c0..b7cb6cf1308590e2723ca287
> > 2cf917edb4036c57 100644
> > > --- a/gcc/tree-vect-patterns.cc
> > > +++ b/gcc/tree-vect-patterns.cc
> > > @@ -5946,8 +5946,45 @@ vect_recog_gcond_pattern (vec_info *vinfo,
> > >
> > > if (code == NE_EXPR
> > > && zerop (rhs)
> > > + && TREE_CODE (lhs) == SSA_NAME
> > > && VECT_SCALAR_BOOLEAN_TYPE_P (scalar_type))
> > > - return NULL;
> > > + {
> > > + stmt_vec_info def_stmt_info = vect_get_internal_def (vinfo, lhs);
> > > + if (!def_stmt_info)
> > > + return NULL;
> > > +
> > > + gimple *lhs_stmt = STMT_VINFO_STMT (def_stmt_info);
> > > + if (!is_gimple_assign (lhs_stmt))
> > > + return NULL;
> > > +
> > > + /* This is normally handled by adjust_bool_pattern, however this
> > > is
> > > + driven from a gimple assign normally and cannot handle gcond.
> > > + BIT_NOT_EXPR are the only ones needing to be lowered for correctness,
> > > + the rest are more optimizations. */
> > > + switch (gimple_assign_rhs_code (lhs_stmt))
> > > + {
> > > + case BIT_NOT_EXPR:
> > > + {
> > > + tree rhs1 = gimple_assign_rhs1 (lhs_stmt);
> > > + tree itype = TREE_TYPE (rhs1);
> > > + tree ilhs = vect_recog_temp_ssa_var (boolean_type_node, NULL);
> > > + gimple *pattern_stmt
> > > + = gimple_build_assign (ilhs, BIT_XOR_EXPR, rhs1,
> > > + build_int_cst (itype, 1));
> > > +
> > > + tree lhs_vtype = get_mask_type_for_scalar_type (vinfo, itype);
> > > + if (lhs_vtype == NULL_TREE)
> > > + return NULL;
> > > +
> > > + append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, lhs_vtype,
> > > + itype);
> > > + lhs = ilhs;
> > > + break;
> > > + }
> > > + default:
> > > + return NULL;
> > > + }
> > > + }
> > >
> > > tree vecitype = get_vectype_for_scalar_type (vinfo, scalar_type);
> > > if (vecitype == NULL_TREE)
> > > @@ -6208,7 +6245,6 @@ vect_recog_bool_pattern (vec_info *vinfo,
> > > return NULL;
> > > }
> > >
> > > -
> > > /* A helper for vect_recog_mask_conversion_pattern. Build
> > > conversion of MASK to a type suitable for masking VECTYPE.
> > > Built statement gets required vectype and is appended to
> > >
> > >
> > >
> > >
> > >
> >
> > --
> > Richard Biener <rguent...@suse.de>
> > SUSE Software Solutions Germany GmbH,
> > Frankenstrasse 146, 90461 Nuernberg, Germany;
> > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
