[AArch64] Allow symbol+offset even if not being used for memory access

Hi,

This patch builds on a previous one that allowed symbol+offset as symbol
references for memory accesses.  It allows us to have symbol+offset even
when no memory access is apparent.

It reduces codesize for cases such as this one:

  int arr[100];

  uint64_t foo (uint64_t a) {
    uint64_t const z = 1234567ll<<32+7;
    uint64_t const y = (uint64_t) &arr[3];
    return y + a + z;
  }

Before the patch, the code looked like this:

    adrp    x2, arr            
    mov     x1, 74217034874880 
    add     x2, x2, :lo12:arr  
    add     x2, x2, 12         
    movk    x1, 2411, lsl 48   
    add     x1, x2, x1         
    add     x0, x1, x0         
    ret                        

Now, it looks like this:
 
    adrp    x1, arr+12
    mov     x2, 74217034874880
    movk    x2, 2411, lsl 48
    add     x1, x1, :lo12:arr+12
    add     x1, x1, x2
    add     x0, x1, x0
    ret

Testing shows no regressions.  OK to commit?


2012-08-31  Ian Bolton  <ian.bolton@arm.com>

	* gcc/config/aarch64/aarch64.md: New pattern.

On 2012-08-31 07:49, Ian Bolton wrote:
> +(define_split
> +  [(set (match_operand:DI 0 "register_operand" "=r")
> +       (const:DI (plus:DI (match_operand:DI 1 "aarch64_valid_symref" "S")
> +                          (match_operand:DI 2 "const_int_operand" "i"))))]
> +  ""
> +  [(set (match_dup 0) (high:DI (const:DI (plus:DI (match_dup 1)
> +  		      	       		 	  (match_dup 2)))))
> +   (set (match_dup 0) (lo_sum:DI (match_dup 0)
> +   		      	         (const:DI (plus:DI (match_dup 1)
> +			       		            (match_dup 2)))))]
> +  ""
> +)

You ought not need this as a separate split, since (CONST ...) should
be handled exactly like (SYMBOL_REF).

Also note that constraints ("=r" etc) aren't used for splits.


r~

> On 2012-08-31 07:49, Ian Bolton wrote:
> > +(define_split
> > +  [(set (match_operand:DI 0 "register_operand" "=r")
> > +       (const:DI (plus:DI (match_operand:DI 1 "aarch64_valid_symref"
> "S")
> > +                          (match_operand:DI 2 "const_int_operand"
> "i"))))]
> > +  ""
> > +  [(set (match_dup 0) (high:DI (const:DI (plus:DI (match_dup 1)
> > +  		      	       		 	  (match_dup 2)))))
> > +   (set (match_dup 0) (lo_sum:DI (match_dup 0)
> > +   		      	         (const:DI (plus:DI (match_dup 1)
> > +			       		            (match_dup 2)))))]
> > +  ""
> > +)
> 
> You ought not need this as a separate split, since (CONST ...) should
> be handled exactly like (SYMBOL_REF).

I see in combine.c that it does get done for a MEM (which is how my
earlier patch worked), but not when it's being used for other reasons
(hence the title of this email).

See below for current code from find_split_point:

    case MEM:
#ifdef HAVE_lo_sum
      /* If we have (mem (const ..)) or (mem (symbol_ref ...)), split it
	 using LO_SUM and HIGH.  */
      if (GET_CODE (XEXP (x, 0)) == CONST
	  || GET_CODE (XEXP (x, 0)) == SYMBOL_REF)
	{
	  enum machine_mode address_mode
	    = targetm.addr_space.address_mode (MEM_ADDR_SPACE (x));

	  SUBST (XEXP (x, 0),
		 gen_rtx_LO_SUM (address_mode,
				 gen_rtx_HIGH (address_mode, XEXP (x, 0)),
				 XEXP (x, 0)));
	  return &XEXP (XEXP (x, 0), 0);
	}
#endif


If I don't use my split pattern, I could alter combine to remove the
requirement that parent is a MEM.

What do you think?

> 
> Also note that constraints ("=r" etc) aren't used for splits.
> 

If I keep the pattern, I will remove the constraints.  Thanks for the
pointers in this regard.


Cheers,
Ian

On 09/06/2012 08:06 AM, Ian Bolton wrote:
> If I don't use my split pattern, I could alter combine to remove the
> requirement that parent is a MEM.
> 
> What do you think?

I merely question the calling out of CONST as special.

Either you've got some pattern that handles SYMBOL_REF
the same way, or you're missing something.


r~

> From: Richard Henderson [mailto:rth@redhat.com]
> On 09/06/2012 08:06 AM, Ian Bolton wrote:
> > If I don't use my split pattern, I could alter combine to remove the
> > requirement that parent is a MEM.
> >
> > What do you think?
> 
> I merely question the calling out of CONST as special.
> 
> Either you've got some pattern that handles SYMBOL_REF
> the same way, or you're missing something.

Oh, I understand now.  Thanks for clarifying.

Some digging has shown me that the transformation keys off
the equivalence, as highlighted below.  It's always phrased in
terms of a const and never a symbol_ref.


after ud_dce:

   6 r82:DI=high(`arr')
   7 r81:DI=r82:DI+low(`arr')
     REG_DEAD: r82:DI
     REG_EQUAL: `arr'
   8 r80:DI=r81:DI+0xc
     REG_DEAD: r81:DI
     REG_EQUAL: const(`arr'+0xc)   <----- this equivalence

after combine:

   7 r80:DI=high(const(`arr'+0xc))
   8 r80:DI=r80:DI+low(const(`arr'+0xc))
     REG_EQUAL: const(`arr'+0xc)   <----- this equivalence


Based on that, and assuming I remove the constraints on the
pattern, would you say the patch is worthy of commit?

Thanks,
Ian

On 09/06/2012 10:19 AM, Ian Bolton wrote:
> Based on that, and assuming I remove the constraints on the
> pattern, would you say the patch is worthy of commit?

Can you send me the test case you were looking at for this?


r~

> Can you send me the test case you were looking at for this?

See attached.  (Most of it is superfluous, but the point is that
we are not using the address to do a memory access.)

Cheers,
Ian