On Mon, 2 Dec 2024, Tamar Christina wrote:
> > -----Original Message-----
> > From: Richard Biener <richard.guent...@gmail.com>
> > Sent: Friday, November 29, 2024 8:57 AM
> > To: Tamar Christina <tamar.christ...@arm.com>
> > Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; rguent...@suse.de;
> > j...@ventanamicro.com
> > Subject: Re: [PATCH 1/2]middle-end: refactor type to be explicit in
> > operand_equal_p [PR114932]
> >
> > On Tue, Aug 20, 2024 at 3:07 PM Tamar Christina <tamar.christ...@arm.com>
> > wrote:
> > >
> > > Hi All,
> > >
> > > This is a refactoring with no expected behavioral change.
> > > The goal with this is to make the type of the expressions being used
> > > explicit.
> > >
> > > I did not change all the recursive calls to operand_equal_p () to recurse
> > > directly to the new function but instead this goes through the top level
> > > call
> > > which re-extracts the types.
> > >
> > > This was done because in most of the cases where we recurse type == arg.
> > > The second patch makes use of this new flexibility to implement an
> > > overload
> > > of operand_equal_p which checks for equality under two's complement.
> > >
> > > Bootstrapped Regtested on aarch64-none-linux-gnu,
> > > arm-none-linux-gnueabihf, x86_64-pc-linux-gnu
> > > -m32, -m64 and no issues.
> > >
> > > Ok for master?
> > >
> > > Thanks,
> > > Tamar
> > >
> > > gcc/ChangeLog:
> > >
> > > PR tree-optimization/114932
> > > * fold-const.cc (operand_compare::operand_equal_p): Split into
> > > one that
> > > takes explicit type parameters and use that in public one.
> > > * fold-const.h (class operand_compare): Add operand_equal_p
> > > private
> > > overload.
> > >
> > > ---
> > > diff --git a/gcc/fold-const.h b/gcc/fold-const.h
> > > index
> > b82ef137e2f2096f86c20df3c7749747e604177e..878545b1148b839e8a8e866f
> > 38e31161f0d116c8 100644
> > > --- a/gcc/fold-const.h
> > > +++ b/gcc/fold-const.h
> > > @@ -273,6 +273,12 @@ protected:
> > > true is returned. Then RET is set to corresponding comparsion
> > > result. */
> > > bool verify_hash_value (const_tree arg0, const_tree arg1, unsigned int
> > > flags,
> > > bool *ret);
> > > +
> > > +private:
> > > + /* Return true if two operands are equal. The flags fields can be used
> > > + to specify OEP flags described in tree-core.h. */
> > > + bool operand_equal_p (tree, const_tree, tree, const_tree,
> > > + unsigned int flags);
> > > };
> > >
> > > #endif // GCC_FOLD_CONST_H
> > > diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> > > index
> > 8908e7381e72cbbf4a8fd96f18cbf4436aba8441..71e82b1d76d4106c7c23c54af
> > 8b35905a1af9f1c 100644
> > > --- a/gcc/fold-const.cc
> > > +++ b/gcc/fold-const.cc
> > > @@ -3156,6 +3156,17 @@ combine_comparisons (location_t loc,
> > > bool
> > > operand_compare::operand_equal_p (const_tree arg0, const_tree arg1,
> > > unsigned int flags)
> > > +{
> > > + return operand_equal_p (TREE_TYPE (arg0), arg0, TREE_TYPE (arg1), arg1,
> > flags);
> > > +}
> > > +
> > > +/* The same as operand_equal_p however the type of ARG0 and ARG1 are
> > assumed to be
> > > + the TYPE0 and TYPE1 respectively. */
> > > +
> > > +bool
> > > +operand_compare::operand_equal_p (tree type0, const_tree arg0,
> > > + tree type1, const_tree arg1,
> >
> > did you try using const_tree for type0/type1?
> >
>
> I did, but types_compatible_p is non-const and it calls
> useless_type_conversion_p
> which is also non-const. Having a look I don't either of those function
> changes
> type so I could change them all to const_tree if you'd like and see what
> shakes out.
>
> It looks like all the calls done in useless_type_conversion_p are already
> const_tree.
>
> Do you want me to propagate the const_tree down?
It looks like a useful cleanup to try making types_compatible_p and
useless_type_conversion_p take const_tree, but separately.
Richard.
> Thanks,
> Tamar
> > > + unsigned int flags)
> > > {
> > > bool r;
> > > if (verify_hash_value (arg0, arg1, flags, &r))
> > > @@ -3166,25 +3177,25 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > >
> > > /* If either is ERROR_MARK, they aren't equal. */
> > > if (TREE_CODE (arg0) == ERROR_MARK || TREE_CODE (arg1) == ERROR_MARK
> > > - || TREE_TYPE (arg0) == error_mark_node
> > > - || TREE_TYPE (arg1) == error_mark_node)
> > > + || type0 == error_mark_node
> > > + || type1 == error_mark_node)
> > > return false;
> > >
> > > /* Similar, if either does not have a type (like a template id),
> > > they aren't equal. */
> > > - if (!TREE_TYPE (arg0) || !TREE_TYPE (arg1))
> > > + if (!type0 || !type1)
> > > return false;
> > >
> > > /* Bitwise identity makes no sense if the values have different
> > > layouts. */
> > > if ((flags & OEP_BITWISE)
> > > - && !tree_nop_conversion_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
> > > + && !tree_nop_conversion_p (type0, type1))
> > > return false;
> > >
> > > /* We cannot consider pointers to different address space equal. */
> > > - if (POINTER_TYPE_P (TREE_TYPE (arg0))
> > > - && POINTER_TYPE_P (TREE_TYPE (arg1))
> > > - && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
> > > - != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
> > > + if (POINTER_TYPE_P (type0)
> > > + && POINTER_TYPE_P (type1)
> > > + && (TYPE_ADDR_SPACE (TREE_TYPE (type0))
> > > + != TYPE_ADDR_SPACE (TREE_TYPE (type1))))
> > > return false;
> > >
> > > /* Check equality of integer constants before bailing out due to
> > > @@ -3211,12 +3222,15 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > >
> > > /* If both types don't have the same precision, then it is not safe
> > > to strip NOPs. */
> > > - if (element_precision (TREE_TYPE (arg0))
> > > - != element_precision (TREE_TYPE (arg1)))
> > > + if (element_precision (type0)
> > > + != element_precision (type1))
> > > return false;
> > >
> > > STRIP_NOPS (arg0);
> > > STRIP_NOPS (arg1);
> > > +
> > > + type0 = TREE_TYPE (arg0);
> > > + type1 = TREE_TYPE (arg1);
> > > }
> > > #if 0
> > > /* FIXME: Fortran FE currently produce ADDR_EXPR of NOP_EXPR. Enable
> > > the
> > > @@ -3275,9 +3289,9 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > >
> > > /* When not checking adddresses, this is needed for conversions and for
> > > COMPONENT_REF. Might as well play it safe and always test this. */
> > > - if (TREE_CODE (TREE_TYPE (arg0)) == ERROR_MARK
> > > - || TREE_CODE (TREE_TYPE (arg1)) == ERROR_MARK
> > > - || (TYPE_MODE (TREE_TYPE (arg0)) != TYPE_MODE (TREE_TYPE (arg1))
> > > + if (TREE_CODE (type0) == ERROR_MARK
> > > + || TREE_CODE (type1) == ERROR_MARK
> > > + || (TYPE_MODE (type0) != TYPE_MODE (type1)
> > > && !(flags & OEP_ADDRESS_OF)))
> > > return false;
> > >
> > > @@ -3364,8 +3378,8 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > return true;
> > >
> > > /* See sem_variable::equals in ipa-icf for a similar approach.
> > > */
> > > - tree typ0 = TREE_TYPE (arg0);
> > > - tree typ1 = TREE_TYPE (arg1);
> > > + tree typ0 = type0;
> > > + tree typ1 = type1;
> >
> > I suppose you should instead eliminate typ0 and typ1.
> >
> > >
> > > if (TREE_CODE (typ0) != TREE_CODE (typ1))
> > > return false;
> > > @@ -3444,8 +3458,8 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > {
> > > CASE_CONVERT:
> > > case FIX_TRUNC_EXPR:
> > > - if (TYPE_UNSIGNED (TREE_TYPE (arg0))
> > > - != TYPE_UNSIGNED (TREE_TYPE (arg1)))
> > > + if (TYPE_UNSIGNED (type0)
> > > + != TYPE_UNSIGNED (type1))
> >
> > it seems this now fits one line
> >
> > > return false;
> > > break;
> > > default:
> > > @@ -3481,12 +3495,12 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > case INDIRECT_REF:
> > > if (!(flags & OEP_ADDRESS_OF))
> > > {
> > > - if (TYPE_ALIGN (TREE_TYPE (arg0))
> > > - != TYPE_ALIGN (TREE_TYPE (arg1)))
> > > + if (TYPE_ALIGN (type0)
> > > + != TYPE_ALIGN (type1))
> >
> > likewise (and maybe elsewhere).
> >
> > Otherwise this looks good. It's OK when 2/2 is approved or OK when doing
> > this
> > without adding a new parameter but adding locals initializing them
> > from arg0/arg1
> > as an intermediate step.
> >
> > Thanks,
> > Richard.
> >
> > > return false;
> > > /* Verify that the access types are compatible. */
> > > - if (TYPE_MAIN_VARIANT (TREE_TYPE (arg0))
> > > - != TYPE_MAIN_VARIANT (TREE_TYPE (arg1)))
> > > + if (TYPE_MAIN_VARIANT (type0)
> > > + != TYPE_MAIN_VARIANT (type1))
> > > return false;
> > > }
> > > flags &= ~OEP_ADDRESS_OF;
> > > @@ -3494,8 +3508,8 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > >
> > > case IMAGPART_EXPR:
> > > /* Require the same offset. */
> > > - if (!operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> > > - TYPE_SIZE (TREE_TYPE (arg1)),
> > > + if (!operand_equal_p (TYPE_SIZE (type0),
> > > + TYPE_SIZE (type1),
> > > flags & ~OEP_ADDRESS_OF))
> > > return false;
> > >
> > > @@ -3509,15 +3523,15 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > if (!(flags & OEP_ADDRESS_OF))
> > > {
> > > /* Require equal access sizes */
> > > - if (TYPE_SIZE (TREE_TYPE (arg0)) != TYPE_SIZE (TREE_TYPE
> > > (arg1))
> > > - && (!TYPE_SIZE (TREE_TYPE (arg0))
> > > - || !TYPE_SIZE (TREE_TYPE (arg1))
> > > - || !operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> > > - TYPE_SIZE (TREE_TYPE (arg1)),
> > > + if (TYPE_SIZE (type0) != TYPE_SIZE (type1)
> > > + && (!TYPE_SIZE (type0)
> > > + || !TYPE_SIZE (type1)
> > > + || !operand_equal_p (TYPE_SIZE (type0),
> > > + TYPE_SIZE (type1),
> > > flags)))
> > > return false;
> > > /* Verify that access happens in similar types. */
> > > - if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE
> > > (arg1)))
> > > + if (!types_compatible_p (type0, type1))
> > > return false;
> > > /* Verify that accesses are TBAA compatible. */
> > > if (!alias_ptr_types_compatible_p
> > > @@ -3529,8 +3543,8 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > != MR_DEPENDENCE_BASE (arg1)))
> > > return false;
> > > /* Verify that alignment is compatible. */
> > > - if (TYPE_ALIGN (TREE_TYPE (arg0))
> > > - != TYPE_ALIGN (TREE_TYPE (arg1)))
> > > + if (TYPE_ALIGN (type0)
> > > + != TYPE_ALIGN (type1))
> > > return false;
> > > }
> > > flags &= ~OEP_ADDRESS_OF;
> > > @@ -3802,16 +3816,16 @@ operand_compare::operand_equal_p (const_tree
> > arg0, const_tree arg1,
> > > indexed in increasing order and form an initial sequence.
> > >
> > > We make no effort to compare nonconstant ones in GENERIC. */
> > > - if (!VECTOR_TYPE_P (TREE_TYPE (arg0))
> > > - || !VECTOR_TYPE_P (TREE_TYPE (arg1)))
> > > + if (!VECTOR_TYPE_P (type0)
> > > + || !VECTOR_TYPE_P (type1))
> > > return false;
> > >
> > > /* Be sure that vectors constructed have the same
> > > representation.
> > > We only tested element precision and modes to match.
> > > Vectors may be BLKmode and thus also check that the number of
> > > parts match. */
> > > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg0)),
> > > - TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg1))))
> > > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (type0),
> > > + TYPE_VECTOR_SUBPARTS (type1)))
> > > return false;
> > >
> > > vec<constructor_elt, va_gc> *v0 = CONSTRUCTOR_ELTS (arg0);
> > >
> > >
> > >
> > >
> > > --
>
--
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)
