Patchwork mem-ref2 merge, core patch

login
register
mail settings
Submitter Richard Guenther
Date June 28, 2010, 10:58 a.m.
Message ID <alpine.LNX.2.00.1006281243490.1429@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/57130/
State New
Headers show

Comments

Richard Guenther - June 28, 2010, 10:58 a.m.
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


+         /* ???  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.
Paolo Bonzini - June 28, 2010, 12:26 p.m.
On 06/28/2010 12:58 PM, Richard Guenther wrote:
> 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.

Thanks, this should likely go in the docs.

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

Then I had understood correctly, thanks. :)  Any idea about the future 
representation of ALIGN_INDIRECT_REF and MISALIGNED_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).

Yes, indeed.

Paolo
Richard Guenther - June 28, 2010, 12:38 p.m.
On Mon, 28 Jun 2010, Paolo Bonzini wrote:

> On 06/28/2010 12:58 PM, Richard Guenther wrote:
> > 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.
> 
> Thanks, this should likely go in the docs.

I'm going to search for a proper place ...

> > > 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).
> 
> Then I had understood correctly, thanks. :)  Any idea about the future
> representation of ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF?

ALIGN_INDIRECT_REF I will replace with

  ptr = ptr & mask;
  MEM_REF [ptr];

thus simply do what expansion replaces it with.  That will allow
straight-forward tracking of alignment via ...

... the replacement for MISALIGNED_INDIRECT_REF.  We have the
long-standing problem of not being able to track alignment
or conservatively assess that of pointers.  My plan is to
embed alignment information in SSA_NAME_PTR_INFO, an
alignment value plus a misalignment value.  Thus,

double x;

char *foo(void)
{
  char *p = &x;  alignment 8, misalign 0
  p++;  alignment 8, misalign 1
  p++;  alignment 8, misalign 2
  ...
}

information filled in by a simple propagator pass.  I'm not
yet sure whether to piggy-back on an existing pass or whether
to invent a new one.  Into-SSA will fill in information from
dereference sites - we will still rely on the frontends to
provide correct pointed-to alignments.  So for

 ... = MEM [(char *)p, 2]

we will conclude that x is aligned to a byte boundary
while for

 ... = MEM [(int *)p, 2]

we will conclude that x is aligned to 4 bytes with misalignment 2.
Similar to what we derive from INDIRECT_REF types at expansion time.
Like when deriving information from INDIRECT_REFs this is somewhat
fragile, so this is only to be done on unoptimized code during
into-SSA.

Basically all the code looking at types in get_pointer_alignment
and set_mem_attributes_minus_bitpos is supposed to vanish and
alignment is to be derived from the SSA_NAME_PTR_INFO only.

Richard.
Jan Hubicka - June 28, 2010, 12:42 p.m.
> On Mon, 28 Jun 2010, Paolo Bonzini wrote:
> 
> > On 06/28/2010 12:58 PM, Richard Guenther wrote:
> > > 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.
> > 
> > Thanks, this should likely go in the docs.
> 
> I'm going to search for a proper place ...
> 
> > > > 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).
> > 
> > Then I had understood correctly, thanks. :)  Any idea about the future
> > representation of ALIGN_INDIRECT_REF and MISALIGNED_INDIRECT_REF?
> 
> ALIGN_INDIRECT_REF I will replace with
> 
>   ptr = ptr & mask;
>   MEM_REF [ptr];
> 
> thus simply do what expansion replaces it with.  That will allow
> straight-forward tracking of alignment via ...
> 
> ... the replacement for MISALIGNED_INDIRECT_REF.  We have the
> long-standing problem of not being able to track alignment
> or conservatively assess that of pointers.  My plan is to
> embed alignment information in SSA_NAME_PTR_INFO, an
> alignment value plus a misalignment value.  Thus,
> 
> double x;
> 
> char *foo(void)
> {
>   char *p = &x;  alignment 8, misalign 0
>   p++;  alignment 8, misalign 1
>   p++;  alignment 8, misalign 2
>   ...
> }

This is definitly important for stringop expansion, especially when we can
track the alignment returned from malloc to be a lot greater than what
type suggest...

Honza
Richard Guenther - June 28, 2010, 1:05 p.m.
On Mon, 28 Jun 2010, Richard Guenther wrote:

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

On a 2nd thought -details isn't really suitable for this.  If in the
end people do not feel too distracted when seeing 'MEM[(int *)&a]'
instead of 'a' then I'd rather disable the above fancy-printing
completely (and adjust all testcases).  As a followup after the
merge.

Richard.
Paolo Bonzini - June 28, 2010, 3:49 p.m.
On 06/28/2010 02:38 PM, Richard Guenther wrote:
> ALIGN_INDIRECT_REF I will replace with
>
>    ptr = ptr & mask;
>    MEM_REF [ptr];
>
> thus simply do what expansion replaces it with.

So you are relying on TER to put together the pieces, right (when it 
makes sense)?  Furthermore, the MEM_REF will have -mask alignment thanks 
to the MISALIGNED_INDIRECT_REF infrastructure, and the misalignment info 
may in the future allow replacing the & with an offset in the MEM_REF. 
Makes sense, thanks!

Regarding the dumping question, it definitely can wait past merge, no 
matter how it is implemented.

Paolo
Richard Guenther - June 28, 2010, 3:53 p.m.
On Mon, 28 Jun 2010, Paolo Bonzini wrote:

> On 06/28/2010 02:38 PM, Richard Guenther wrote:
> > ALIGN_INDIRECT_REF I will replace with
> > 
> >    ptr = ptr & mask;
> >    MEM_REF [ptr];
> > 
> > thus simply do what expansion replaces it with.
> 
> So you are relying on TER to put together the pieces, right (when it makes
> sense)?

Well, I'm just expanding it on trees early.  In expr.c we have

    case MISALIGNED_INDIRECT_REF:
    case ALIGN_INDIRECT_REF:
    case INDIRECT_REF:
      {
...
        if (code == ALIGN_INDIRECT_REF)
          {
            int align = TYPE_ALIGN_UNIT (type);
            op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
            op0 = memory_address_addr_space (mode, op0, as);
          }

we seem to rely on combine or whatever to do fancy things (I tried
to figure out where exactly things should be optimized but I even
failed to see in which cases exactly we end up generating
ALIGN_INDIRECT_REF in the first place ...)

> Furthermore, the MEM_REF will have -mask alignment thanks to the
> MISALIGNED_INDIRECT_REF infrastructure, and the misalignment info may in the
> future allow replacing the & with an offset in the MEM_REF. Makes sense,
> thanks!

Right.

> Regarding the dumping question, it definitely can wait past merge, no matter
> how it is implemented.

It's definitely useful for debugging.

Richard.
Paolo Bonzini - June 28, 2010, 4:34 p.m.
On 06/28/2010 05:53 PM, Richard Guenther wrote:
> On Mon, 28 Jun 2010, Paolo Bonzini wrote:
>
>> On 06/28/2010 02:38 PM, Richard Guenther wrote:
>>> ALIGN_INDIRECT_REF I will replace with
>>>
>>>     ptr = ptr&  mask;
>>>     MEM_REF [ptr];
>>>
>>> thus simply do what expansion replaces it with.
>>
>> So you are relying on TER to put together the pieces, right (when it makes
>> sense)?
>
> Well, I'm just expanding it on trees early.  In expr.c we have
>
>      case MISALIGNED_INDIRECT_REF:
>      case ALIGN_INDIRECT_REF:
>      case INDIRECT_REF:
>        {
> ...
>          if (code == ALIGN_INDIRECT_REF)
>            {
>              int align = TYPE_ALIGN_UNIT (type);
>              op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
>              op0 = memory_address_addr_space (mode, op0, as);
>            }
>
> we seem to rely on combine or whatever to do fancy things

No, the AND is getting into the expansion directly if it is a legitimate 
address.

> (I tried
> to figure out where exactly things should be optimized but I even
> failed to see in which cases exactly we end up generating
> ALIGN_INDIRECT_REF in the first place ...)

I think PPC vectorization uses it.

Paolo
Richard Guenther - June 29, 2010, 9:03 a.m.
On Mon, 28 Jun 2010, Paolo Bonzini wrote:

> On 06/28/2010 05:53 PM, Richard Guenther wrote:
> > On Mon, 28 Jun 2010, Paolo Bonzini wrote:
> > 
> > > On 06/28/2010 02:38 PM, Richard Guenther wrote:
> > > > ALIGN_INDIRECT_REF I will replace with
> > > > 
> > > >     ptr = ptr&  mask;
> > > >     MEM_REF [ptr];
> > > > 
> > > > thus simply do what expansion replaces it with.
> > > 
> > > So you are relying on TER to put together the pieces, right (when it makes
> > > sense)?
> > 
> > Well, I'm just expanding it on trees early.  In expr.c we have
> > 
> >      case MISALIGNED_INDIRECT_REF:
> >      case ALIGN_INDIRECT_REF:
> >      case INDIRECT_REF:
> >        {
> > ...
> >          if (code == ALIGN_INDIRECT_REF)
> >            {
> >              int align = TYPE_ALIGN_UNIT (type);
> >              op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
> >              op0 = memory_address_addr_space (mode, op0, as);
> >            }
> > 
> > we seem to rely on combine or whatever to do fancy things
> 
> No, the AND is getting into the expansion directly if it is a legitimate
> address.

Which is a hack, right?  With the plan I have in mind we'd have

  ptr &= 3;  // align the pointer
  .. = *p;   // aligned load

which should expand fine (to an AND and to an aligned load).  Then
combine can combine those two insns just fine to the aligning load.

> > (I tried
> > to figure out where exactly things should be optimized but I even
> > failed to see in which cases exactly we end up generating
> > ALIGN_INDIRECT_REF in the first place ...)
> 
> I think PPC vectorization uses it.

Of course - that much I figured out by simple grepping ;)  I just
didn't figure out why people thought it was a great idea to
invent ALIGN_INDIRECT_REF when a simple BIT_AND_EXPR on the tree-level
would do (not?).

Thus it's now my part to find out the hard way ... but maybe
Ira has an idea?

Thanks,
Richard.
Paolo Bonzini - June 29, 2010, 9:10 a.m.
On 06/29/2010 11:03 AM, Richard Guenther wrote:
>>>           if (code == ALIGN_INDIRECT_REF)
>>>             {
>>>               int align = TYPE_ALIGN_UNIT (type);
>>>               op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
>>>               op0 = memory_address_addr_space (mode, op0, as);
>>>             }
>>>
>>> we seem to rely on combine or whatever to do fancy things
>>
>> No, the AND is getting into the expansion directly if it is a legitimate
>> address.
>
> Which is a hack, right?  With the plan I have in mind we'd have
>
>    ptr&= 3;  // align the pointer
>    .. = *p;   // aligned load
>
> which should expand fine (to an AND and to an aligned load).  Then
> combine can combine those two insns just fine to the aligning load.

Not necessarily a hack; I could say that a BIT_AND_EXPR on a pointer, 
with a pointer-typed operand 1 0xFFFFFFF0, is also a hack...

But if TER or fwprop can create the addresses already, that's fine by me.

Paolo
Richard Guenther - June 29, 2010, 11:59 a.m.
On Tue, 29 Jun 2010, Richard Guenther wrote:

> On Mon, 28 Jun 2010, Paolo Bonzini wrote:
> 
> > On 06/28/2010 05:53 PM, Richard Guenther wrote:
> > > On Mon, 28 Jun 2010, Paolo Bonzini wrote:
> > > 
> > > > On 06/28/2010 02:38 PM, Richard Guenther wrote:
> > > > > ALIGN_INDIRECT_REF I will replace with
> > > > > 
> > > > >     ptr = ptr&  mask;
> > > > >     MEM_REF [ptr];
> > > > > 
> > > > > thus simply do what expansion replaces it with.
> > > > 
> > > > So you are relying on TER to put together the pieces, right (when it makes
> > > > sense)?
> > > 
> > > Well, I'm just expanding it on trees early.  In expr.c we have
> > > 
> > >      case MISALIGNED_INDIRECT_REF:
> > >      case ALIGN_INDIRECT_REF:
> > >      case INDIRECT_REF:
> > >        {
> > > ...
> > >          if (code == ALIGN_INDIRECT_REF)
> > >            {
> > >              int align = TYPE_ALIGN_UNIT (type);
> > >              op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
> > >              op0 = memory_address_addr_space (mode, op0, as);
> > >            }
> > > 
> > > we seem to rely on combine or whatever to do fancy things
> > 
> > No, the AND is getting into the expansion directly if it is a legitimate
> > address.
> 
> Which is a hack, right?  With the plan I have in mind we'd have
> 
>   ptr &= 3;  // align the pointer
>   .. = *p;   // aligned load
> 
> which should expand fine (to an AND and to an aligned load).  Then
> combine can combine those two insns just fine to the aligning load.
> 
> > > (I tried
> > > to figure out where exactly things should be optimized but I even
> > > failed to see in which cases exactly we end up generating
> > > ALIGN_INDIRECT_REF in the first place ...)
> > 
> > I think PPC vectorization uses it.
> 
> Of course - that much I figured out by simple grepping ;)  I just
> didn't figure out why people thought it was a great idea to
> invent ALIGN_INDIRECT_REF when a simple BIT_AND_EXPR on the tree-level
> would do (not?).
> 
> Thus it's now my part to find out the hard way ... but maybe
> Ira has an idea?

Btw, the following hack at least passes the C vectorizer testsuite on
powerpc64 (and no rliwnm instructions are generated).  So I plan to
continue down this road to eliminate ALIGN_INDIRECT_REF.

Richard.

Index: gcc/expr.c
===================================================================
*** gcc/expr.c	(revision 161488)
--- gcc/expr.c	(working copy)
*************** expand_expr_real_1 (tree exp, rtx target
*** 8677,8688 ****
        return expand_constructor (exp, target, modifier, false);
  
      case MISALIGNED_INDIRECT_REF:
-     case ALIGN_INDIRECT_REF:
      case INDIRECT_REF:
        {
  	tree exp1 = treeop0;
  	addr_space_t as = ADDR_SPACE_GENERIC;
- 	enum machine_mode address_mode = Pmode;
  
  	if (modifier != EXPAND_WRITE)
  	  {
--- 8677,8686 ----
*************** expand_expr_real_1 (tree exp, rtx target
*** 8696,8714 ****
  	if (POINTER_TYPE_P (TREE_TYPE (exp1)))
  	  {
  	    as = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (exp1)));
- 	    address_mode = targetm.addr_space.address_mode (as);
  	  }
  
  	op0 = expand_expr (exp1, NULL_RTX, VOIDmode, EXPAND_SUM);
  	op0 = memory_address_addr_space (mode, op0, as);
  
- 	if (code == ALIGN_INDIRECT_REF)
- 	  {
- 	    int align = TYPE_ALIGN_UNIT (type);
- 	    op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
- 	    op0 = memory_address_addr_space (mode, op0, as);
- 	  }
- 
  	temp = gen_rtx_MEM (mode, op0);
  
  	set_mem_attributes (temp, exp, 0);
--- 8694,8704 ----
*************** expand_expr_real_1 (tree exp, rtx target
*** 8773,8778 ****
--- 8763,8769 ----
  	  = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 1))));
  	enum machine_mode address_mode;
  	tree base = TREE_OPERAND (exp, 0);
+ 	gimple def_stmt;
  	/* Handle expansion of non-aliased memory with non-BLKmode.  That
  	   might end up in a register.  */
  	if (TREE_CODE (base) == ADDR_EXPR)
*************** expand_expr_real_1 (tree exp, rtx target
*** 8815,8822 ****
  	      }
  	  }
  	address_mode = targetm.addr_space.address_mode (as);
! 	op0 = expand_expr (TREE_OPERAND (exp, 0), NULL_RTX, address_mode,
! 			   EXPAND_NORMAL);
  	if (!integer_zerop (TREE_OPERAND (exp, 1)))
  	  {
  	    rtx off;
--- 8806,8817 ----
  	      }
  	  }
  	address_mode = targetm.addr_space.address_mode (as);
! 	base = TREE_OPERAND (exp, 0);
! 	if ((def_stmt = get_def_for_expr (base, BIT_AND_EXPR)))
! 	  base = build2 (BIT_AND_EXPR, TREE_TYPE (base),
! 			 gimple_assign_rhs1 (def_stmt),
! 			 gimple_assign_rhs2 (def_stmt));
! 	op0 = expand_expr (base, NULL_RTX, address_mode, EXPAND_NORMAL);
  	if (!integer_zerop (TREE_OPERAND (exp, 1)))
  	  {
  	    rtx off;
Index: gcc/tree-vect-data-refs.c
===================================================================
*** gcc/tree-vect-data-refs.c	(revision 161488)
--- gcc/tree-vect-data-refs.c	(working copy)
*************** vect_setup_realignment (gimple stmt, gim
*** 3178,3184 ****
        vec_dest = vect_create_destination_var (scalar_dest, vectype);
        ptr = vect_create_data_ref_ptr (stmt, loop_for_initial_load, NULL_TREE,
  				      &init_addr, &inc, true, &inv_p);
!       data_ref = build1 (ALIGN_INDIRECT_REF, vectype, ptr);
        new_stmt = gimple_build_assign (vec_dest, data_ref);
        new_temp = make_ssa_name (vec_dest, new_stmt);
        gimple_assign_set_lhs (new_stmt, new_temp);
--- 3178,3192 ----
        vec_dest = vect_create_destination_var (scalar_dest, vectype);
        ptr = vect_create_data_ref_ptr (stmt, loop_for_initial_load, NULL_TREE,
  				      &init_addr, &inc, true, &inv_p);
!       new_stmt = gimple_build_assign_with_ops (BIT_AND_EXPR,
! 					       NULL_TREE, ptr,
! 					       build_int_cst (TREE_TYPE (ptr),
! 							      -(HOST_WIDE_INT)TYPE_ALIGN_UNIT ((vectype))));
!       new_temp = make_ssa_name (SSA_NAME_VAR (ptr), new_stmt);
!       gimple_assign_set_lhs (new_stmt, new_temp);
!       new_bb = gsi_insert_on_edge_immediate (pe, new_stmt);
!       gcc_assert (!new_bb);
!       data_ref = build_simple_mem_ref (new_temp);
        new_stmt = gimple_build_assign (vec_dest, data_ref);
        new_temp = make_ssa_name (vec_dest, new_stmt);
        gimple_assign_set_lhs (new_stmt, new_temp);
Index: gcc/tree-vect-stmts.c
===================================================================
*** gcc/tree-vect-stmts.c	(revision 161488)
--- gcc/tree-vect-stmts.c	(working copy)
*************** vectorizable_load (gimple stmt, gimple_s
*** 3684,3690 ****
  						dr_explicit_realign,
  						dataref_ptr, NULL);
  
! 		data_ref = build1 (ALIGN_INDIRECT_REF, vectype, dataref_ptr);
  		vec_dest = vect_create_destination_var (scalar_dest, vectype);
  		new_stmt = gimple_build_assign (vec_dest, data_ref);
  		new_temp = make_ssa_name (vec_dest, new_stmt);
--- 3684,3697 ----
  						dr_explicit_realign,
  						dataref_ptr, NULL);
  
! 		new_stmt = gimple_build_assign_with_ops (BIT_AND_EXPR,
! 							 NULL_TREE, dataref_ptr,
! 							 build_int_cst (TREE_TYPE (dataref_ptr),
! 									-(HOST_WIDE_INT)TYPE_ALIGN_UNIT ((vectype))));
! 		ptr = make_ssa_name (SSA_NAME_VAR (dataref_ptr), new_stmt);
! 		gimple_assign_set_lhs (new_stmt, ptr);
! 		vect_finish_stmt_generation (stmt, new_stmt, gsi);
! 		data_ref = build_simple_mem_ref (ptr);
  		vec_dest = vect_create_destination_var (scalar_dest, vectype);
  		new_stmt = gimple_build_assign (vec_dest, data_ref);
  		new_temp = make_ssa_name (vec_dest, new_stmt);
*************** vectorizable_load (gimple stmt, gimple_s
*** 3697,3707 ****
  		bump = size_binop (MULT_EXPR, vs_minus_1,
  				   TYPE_SIZE_UNIT (scalar_type));
  		ptr = bump_vector_ptr (dataref_ptr, NULL, gsi, stmt, bump);
! 	        data_ref = build1 (ALIGN_INDIRECT_REF, vectype, ptr);
  	        break;
  	      }
  	    case dr_explicit_realign_optimized:
! 	      data_ref = build1 (ALIGN_INDIRECT_REF, vectype, dataref_ptr);
  	      break;
  	    default:
  	      gcc_unreachable ();
--- 3704,3728 ----
  		bump = size_binop (MULT_EXPR, vs_minus_1,
  				   TYPE_SIZE_UNIT (scalar_type));
  		ptr = bump_vector_ptr (dataref_ptr, NULL, gsi, stmt, bump);
! 		new_stmt = gimple_build_assign_with_ops (BIT_AND_EXPR,
! 							 NULL_TREE, ptr,
! 							 build_int_cst (TREE_TYPE (ptr),
! 									-(HOST_WIDE_INT)TYPE_ALIGN_UNIT ((vectype))));
! 		ptr = make_ssa_name (SSA_NAME_VAR (dataref_ptr), new_stmt);
! 		gimple_assign_set_lhs (new_stmt, ptr);
! 		vect_finish_stmt_generation (stmt, new_stmt, gsi);
! 	        data_ref = build_simple_mem_ref (ptr);
  	        break;
  	      }
  	    case dr_explicit_realign_optimized:
! 	      new_stmt = gimple_build_assign_with_ops (BIT_AND_EXPR,
! 						       NULL_TREE, dataref_ptr,
! 						       build_int_cst (TREE_TYPE (dataref_ptr),
! 								      -(HOST_WIDE_INT)TYPE_ALIGN_UNIT ((vectype))));
! 	      new_temp = make_ssa_name (SSA_NAME_VAR (dataref_ptr), new_stmt);
! 	      gimple_assign_set_lhs (new_stmt, new_temp);
! 	      vect_finish_stmt_generation (stmt, new_stmt, gsi);
! 	      data_ref = build_simple_mem_ref (new_temp);
  	      break;
  	    default:
  	      gcc_unreachable ();
Paolo Bonzini - June 29, 2010, 12:54 p.m.
On 06/29/2010 01:59 PM, Richard Guenther wrote:
> Btw, the following hack at least passes the C vectorizer testsuite on
> powerpc64 (and no rliwnm instructions are generated).  So I plan to
> continue down this road to eliminate ALIGN_INDIRECT_REF.

Cool, it looks sane from the expander point of view.

Paolo
Ira Rosen - June 30, 2010, 6:10 a.m.
Richard Guenther <rguenther@suse.de> wrote on 29/06/2010 12:03:58 PM:

> From: Richard Guenther <rguenther@suse.de>
> To: Paolo Bonzini <bonzini@gnu.org>
> Cc: gcc-patches@gcc.gnu.org, Ira Rosen/Haifa/IBM@IBMIL
> Date: 29/06/2010 12:05 PM
> Subject: Re: [PATCH] mem-ref2 merge, core patch
>
> On Mon, 28 Jun 2010, Paolo Bonzini wrote:
>
> > On 06/28/2010 05:53 PM, Richard Guenther wrote:
> > > On Mon, 28 Jun 2010, Paolo Bonzini wrote:
> > >
> > > > On 06/28/2010 02:38 PM, Richard Guenther wrote:
> > > > > ALIGN_INDIRECT_REF I will replace with
> > > > >
> > > > >     ptr = ptr&  mask;
> > > > >     MEM_REF [ptr];
> > > > >
> > > > > thus simply do what expansion replaces it with.
> > > >
> > > > So you are relying on TER to put together the pieces, right
> (when it makes
> > > > sense)?
> > >
> > > Well, I'm just expanding it on trees early.  In expr.c we have
> > >
> > >      case MISALIGNED_INDIRECT_REF:
> > >      case ALIGN_INDIRECT_REF:
> > >      case INDIRECT_REF:
> > >        {
> > > ...
> > >          if (code == ALIGN_INDIRECT_REF)
> > >            {
> > >              int align = TYPE_ALIGN_UNIT (type);
> > >              op0 = gen_rtx_AND (address_mode, op0, GEN_INT (-align));
> > >              op0 = memory_address_addr_space (mode, op0, as);
> > >            }
> > >
> > > we seem to rely on combine or whatever to do fancy things
> >
> > No, the AND is getting into the expansion directly if it is a
legitimate
> > address.
>
> Which is a hack, right?  With the plan I have in mind we'd have
>
>   ptr &= 3;  // align the pointer
>   .. = *p;   // aligned load
>
> which should expand fine (to an AND and to an aligned load).  Then
> combine can combine those two insns just fine to the aligning load.
>
> > > (I tried
> > > to figure out where exactly things should be optimized but I even
> > > failed to see in which cases exactly we end up generating
> > > ALIGN_INDIRECT_REF in the first place ...)
> >
> > I think PPC vectorization uses it.
>
> Of course - that much I figured out by simple grepping ;)  I just
> didn't figure out why people thought it was a great idea to
> invent ALIGN_INDIRECT_REF when a simple BIT_AND_EXPR on the tree-level
> would do (not?).
>
> Thus it's now my part to find out the hard way ... but maybe
> Ira has an idea?

Sorry, I don't know. Here is a link to the relevant patch, but it doesn't
help much http://gcc.gnu.org/ml/gcc-patches/2004-09/msg01522.html.

Ira

>
> Thanks,
> Richard.

Patch

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.