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..878545b1148b839e8a8e866f38e31161f0d116c8 > 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..71e82b1d76d4106c7c23c54af8b35905a1af9f1c > 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? > + 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); > > > > > --