On Wed, 7 Oct 2015, Jan Hubicka wrote:
> > > I also disabled type matching done by operand_equal_p and cleaned up the
> > > conditional of MEM_REF into multiple ones - for example it was passing
> > > OEP_ADDRESS_OF when comparing TYPE_SIZE which is quite a nonsense.
> > >
> > > I wonder what to do about OPE_CONSTANT_ADDRESS_OF. This flag does not
> > > seem
> > > to be used at all in current tree nor documented somehow.
> >
> > It is used and (un-)documented as OEP_ADDRESS_OF, see the ADDR_EXPR case:
> >
> > case ADDR_EXPR:
> > return operand_equal_p (TREE_OPERAND (arg0, 0), TREE_OPERAND (arg1, 0),
> > TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)
> > ? OEP_CONSTANT_ADDRESS_OF | OEP_ADDRESS_OF : 0);
> >
> > So it's OEP_ADDRESS_OF but for constant addresses.
>
> Yep, I noticed it, but just after reading the sources for few times. The
> use is well hidden :)
>
> Here is updated patch adding Richard's feedback and also making most of
> OEP_ADDRESS_OF special cases to also handle OEP_CONSTANT_ADDRESS_OF.
> OEP_CONSTANT_ADDRESS_OF means we care about address and we know the whole
> expr is constant, so it is stronger than OEP_ADDRESS_OF.
Yeah, we always set OEP_ADDRESS_OF when we set OEP_CONSTANT_ADDRESS_OF
though ...
> I also added documentation and cleaned up handling of ADDR_EXPR. There are
> two
> cases handing ADDR_EXPR.
> One handles TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1) and the other the
> rest of cases, so we do not need the conditional in the code quoted above.
> This will hopefully make it more obvious when the OEP_CONSTANT_ADDRESS_OF
> is set and used.
>
> I also added sanity check that OEP_ADDRESSOF|OEP_CONSTANT_ADDRESS_OF is not
> used for INTEGER_CST and NOP_EXPR. There are many other cases where we can't
> take address, but this seems strong enough to catch the wrong recursion which
> forgets to clear the flag and forced me to fix the OEP_CONSTANT_ADDRESS_OF
> handling.
>
> I wonder if the INDIRECT_REF also needs the TBAA check that we do for MEM_REF?
> While we lower that early, I think we can still unify the code like in case
> of
> cond ? ref_alias_set_1 : ref_alias_set_2
I believe that INDIRECT_REFs have to be "type safe", that is,
TREE_TYPE (ref) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (ref, 0))) and
thus in theory type checks should catch mismatches. OTOH we only
have
/* 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)))
return 0;
and thus even miss basic TYPE_SIZE checks here... Thus
struct S { int i[3]; };
struct R { int i[7]; };
cond ? *(struct S *)p : *(struct R *)p;
will probably unify (well, hopefully the FE will complain about
the size mismatch, but the INDIRECT_REFs will be operand_equal_p).
Then we also do STRIP_NOPS on the pointer types which will strip
typedef int myint __attribute__((may_alias));
int *p;
cond ? *(myint *)p : *p;
So yeah, the INDIRECT_REF case looks very suspicious.
> Bootstrapped/regtested x86_64-linux, OK?
Few comments below
> Honza
>
> * fold-const.c (operand_equal_p): Document OEP_ADDRESS_OF
> and OEP_CONSTANT_ADDRESS_OF; make most of OEP_ADDRESS_OF
> special cases to also handle OEP_CONSTANT_ADDRESS_OF; skip
> type checking for OPE_*ADDRESS_OF.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c (revision 228131)
> +++ fold-const.c (working copy)
> @@ -2691,7 +2691,12 @@ combine_comparisons (location_t loc,
>
> If OEP_PURE_SAME is set, then pure functions with identical arguments
> are considered the same. It is used when the caller has other ways
> - to ensure that global memory is unchanged in between. */
> + to ensure that global memory is unchanged in between.
> +
> + If OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF is set, we are actually
> comparing
> + addresses of objects, not values of expressions. OEP_CONSTANT_ADDRESS_OF
> + is used for ADDR_EXPR with TREE_CONSTANT flag set and we further ignore
> + any side effects on SAVE_EXPRs then. */
>
> int
> operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
> @@ -2710,31 +2715,48 @@ operand_equal_p (const_tree arg0, const_
> /* Check equality of integer constants before bailing out due to
> precision differences. */
> if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
> - return tree_int_cst_equal (arg0, arg1);
> + {
> + /* Address of INTEGER_CST is not defined; check that we did not forget
> + to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags. */
> + gcc_checking_assert (!(flags
> + & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
> + return tree_int_cst_equal (arg0, arg1);
> + }
>
> - /* If both types don't have the same signedness, then we can't consider
> - them equal. We must check this before the STRIP_NOPS calls
> - because they may change the signedness of the arguments. As pointers
> - strictly don't have a signedness, require either two pointers or
> - two non-pointers as well. */
> - if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
> - || POINTER_TYPE_P (TREE_TYPE (arg0)) != POINTER_TYPE_P (TREE_TYPE
> (arg1)))
> - return 0;
> + if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)))
> + {
> + /* If both types don't have the same signedness, then we can't consider
> + them equal. We must check this before the STRIP_NOPS calls
> + because they may change the signedness of the arguments. As pointers
> + strictly don't have a signedness, require either two pointers or
> + two non-pointers as well. */
> + if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE
> (arg1))
> + || POINTER_TYPE_P (TREE_TYPE (arg0))
> + != POINTER_TYPE_P (TREE_TYPE (arg1)))
> + return 0;
>
> - /* 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)))))
> - return 0;
> + /* 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)))))
> + return 0;
>
> - /* 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)))
> - return 0;
> + /* 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)))
> + return 0;
>
> - STRIP_NOPS (arg0);
> - STRIP_NOPS (arg1);
> + STRIP_NOPS (arg0);
> + STRIP_NOPS (arg1);
> + }
> + else
> + /* Addresses of NOP_EXPR (and many other things) are not well defined.
> + Check that we did not forget to drop the
> + OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags. */
> + gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
> + && TREE_CODE (arg1) != NOP_EXPR);
Use ! CONVERT_EXPR_P (arg0) && ! CONVERT_EXPR_P (arg1)
> /* In case both args are comparisons but with different comparison
> code, try to swap the comparison operands of one arg to produce
> @@ -2757,7 +2779,7 @@ operand_equal_p (const_tree arg0, const_
> /* NOP_EXPR and CONVERT_EXPR are considered equal. */
> if (CONVERT_EXPR_P (arg0) && CONVERT_EXPR_P (arg1))
> ;
> - else if (flags & OEP_ADDRESS_OF)
> + else if (flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
> {
> /* If we are interested in comparing addresses ignore
> MEM_REF wrappings of the base that can appear just for
> @@ -2858,9 +2880,10 @@ operand_equal_p (const_tree arg0, const_
> TREE_STRING_LENGTH (arg0)));
>
> case ADDR_EXPR:
> + /* Be sure we pass right ADDRESS_OF flag. */
> + gcc_checking_assert (TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1));
Well, it's guarded with if (TREE_CONSTANT (arg0) && ... so the
assert is quite pointless.
> return operand_equal_p (TREE_OPERAND (arg0, 0), TREE_OPERAND (arg1, 0),
> - TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)
> - ? OEP_CONSTANT_ADDRESS_OF | OEP_ADDRESS_OF : 0);
> + OEP_CONSTANT_ADDRESS_OF);
> default:
> break;
> }
> @@ -2922,7 +2945,7 @@ operand_equal_p (const_tree arg0, const_
> switch (TREE_CODE (arg0))
> {
> case INDIRECT_REF:
> - if (!(flags & OEP_ADDRESS_OF)
> + if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
> && (TYPE_ALIGN (TREE_TYPE (arg0))
> != TYPE_ALIGN (TREE_TYPE (arg1))))
> return 0;
> @@ -2935,27 +2958,34 @@ operand_equal_p (const_tree arg0, const_
>
> case TARGET_MEM_REF:
> case MEM_REF:
> - /* Require equal access sizes, and similar pointer types.
> - We can have incomplete types for array references of
> - variable-sized arrays from the Fortran frontend
> - though. Also verify the types are compatible. */
> - 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)),
> flags)))
> - && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
> - && ((flags & OEP_ADDRESS_OF)
> - || (alias_ptr_types_compatible_p
> - (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> - TREE_TYPE (TREE_OPERAND (arg1, 1)))
> - && (MR_DEPENDENCE_CLIQUE (arg0)
> - == MR_DEPENDENCE_CLIQUE (arg1))
> - && (MR_DEPENDENCE_BASE (arg0)
> - == MR_DEPENDENCE_BASE (arg1))
> - && (TYPE_ALIGN (TREE_TYPE (arg0))
> - == TYPE_ALIGN (TREE_TYPE (arg1)))))))
> - return 0;
> + if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_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)),
> + flags)))
> + return 0;
> + /* Verify that access happens in similar types. */
> + if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
> + return 0;
> + /* Verify that accesses are TBAA compatible. */
> + if (flag_strict_aliasing
> + && (!alias_ptr_types_compatible_p
> + (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> + TREE_TYPE (TREE_OPERAND (arg1, 1)))
> + || (MR_DEPENDENCE_CLIQUE (arg0)
> + != MR_DEPENDENCE_CLIQUE (arg1))
> + || (MR_DEPENDENCE_BASE (arg0)
> + != MR_DEPENDENCE_BASE (arg1))))
> + return 0;
> + /* Verify that alignment is compatible. */
> + if (TYPE_ALIGN (TREE_TYPE (arg0))
> + != TYPE_ALIGN (TREE_TYPE (arg1)))
> + return 0;
> + }
> flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
> return (OP_SAME (0) && OP_SAME (1)
> /* TARGET_MEM_REF require equal extra operands. */
> @@ -3001,6 +3031,8 @@ operand_equal_p (const_tree arg0, const_
> switch (TREE_CODE (arg0))
> {
> case ADDR_EXPR:
> + /* Be sure we pass right ADDRESS_OF flag. */
> + gcc_checking_assert (!(TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)));
Likewise.
Otherwise the patch looks ok.
Thanks,
Richard.
> return operand_equal_p (TREE_OPERAND (arg0, 0),
> TREE_OPERAND (arg1, 0),
> flags | OEP_ADDRESS_OF);
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
