Re: [PATCH] mem-ref2 merge, core patch

[Richard Guenther <rguenther at suse dot de>]

On Mon, 28 Jun 2010, Paolo Bonzini wrote:

> On 06/25/2010 01:47 PM, Richard Guenther wrote:
> > +	if (integer_zerop (TREE_OPERAND (node, 1))
> > +	    /* Same pointer types, but ignoring POINTER_TYPE vs.
> > +	       REFERENCE_TYPE.  */
> > +	&&  (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 0)))
> > +		== TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))
> > +	&&  (TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 0)))
> > +		== TYPE_MODE (TREE_TYPE (TREE_OPERAND (node, 1))))
> > +	&&  (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node, 0)))
> > +		== TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (TREE_OPERAND (node,
> > 1))))
> > +	&&  (TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 0)))
> > +		== TYPE_QUALS (TREE_TYPE (TREE_OPERAND (node, 1))))
> > +	    /* Same value types ignoring qualifiers.  */
> > +	&&  (TYPE_MAIN_VARIANT (TREE_TYPE (node))
> > +		== TYPE_MAIN_VARIANT
> > +		    (TREE_TYPE (TREE_TYPE (TREE_OPERAND (node, 1))))))
> > +	  {
> > +	    if (TREE_CODE (TREE_OPERAND (node, 0)) != ADDR_EXPR)
> > +	      {
> > +		pp_string (buffer, "*");
> > +		dump_generic_node (buffer, TREE_OPERAND (node, 0),
> > +				   spc, flags, false);
> > +	      }
> 
> Maybe avoid the magic when detailed dumping is active?

The magic is mostly to make the testsuite part of the patch smaller,
which also means that if we change the above with -details some
testcases may start failing.  I'll check, the suggestion is good
I think (so if the resulting change in testresults is small I'll
probably go for it).

> What exactly are the rules for having an INDIRECT_REF or a chain of
> handled_component_p instead of a MEM_REF?

INDIRECT_REF is not allowed in gimple.  You can have bare decls or
MEM_REF, and both can be wrapped inside a handled_component_p chain.
(There also still exist ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF
created by the vectorizer - I will do followup patches to remove those
as well)

Basically MEM_REF can appear wherever an INDIRECT_REF or a decl could 
previously apper.

> Would it make sense to change the other INDIRECT_REF_P types into flags on
> MEM_REF?

INDIRECT_REF_P will be no longer necessary on gimple after I fixed
up the vectorizer to not use ALIGN_INDIRECT_REF and 
MISALIGNED_INDIRECT_REF.  Where it currently appears it is solely
because of those two (I didn't bother to change it to
tests for ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF excluding
INDIRECT_REF).

> Can you expand on the (several) "#if 0/FIXME" comments?

They should have been fixed by followup patches (I just noticed them
while writing the changelog).  Remaining FIXMEs are

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c     (.../trunk)     (revision 161367)
+++ gcc/dwarf2out.c     (.../branches/mem-ref2) (revision 161369)
@@ -15159,6 +15159,11 @@ loc_list_from_tree (tree loc, int want_a
       }
       break;

+    case MEM_REF:
+      /* ??? FIXME.  */
+      if (!integer_zerop (TREE_OPERAND (loc, 1)))
+       return 0;
+      /* Fallthru.  */

I have no idea what to do here (no clue about dwarf).  Similar
for the expand_debug_expr case.

+         /* ???  FIXME.  We should always fold these.  */
+         && !CONSTANT_CLASS_P (TREE_OPERAND (TREE_OPERAND (expr, 0), 0)))

Is not true.  I removed the FIXME in a followup.

Several ???s just mark possible improvements (or rather highlights
places where we trade optimization for correctness, compared to
before MEM_REF).  Like in tree-ssa-alias.c.

There are probably issues in the RTL/expansion parts of the patch,
mostly regarding to possibly not running into optimized code-paths.
Expansion of assignments is really an interesting mystery.  So
I'd appreciate 2nd looks in that area (though I think all improvements
or fixes can be done as a followup after the merge).

@@ -3269,13 +3272,16 @@ gimplify_init_ctor_preeval_1 (tree *tp,
   /* If the constructor component is a call, determine if it can hide a
-     potential overlap with the lhs through an INDIRECT_REF like above.  
*/
+     potential overlap with the lhs through an INDIRECT_REF like above.
+     ??? Ugh - this is completely broken.  In fact this whole analysis
+     doesn't look conservative.  */
   if (TREE_CODE (t) == CALL_EXPR)

well - this comment just hints at that the following code looks
broken to me.  It falls afoul to the same wrong assumptions that
made escape analysis in 4.3 and before completely broken.

I think that were all FIXME/???s in the core patch.

Thanks,
Richard.