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);
>
>
>
>
> --

Reply via email to