Re: [PATCH AArch64]Handle REG+REG+CONST and REG+NON_REG+CONST in legitimize address

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 01/12/15 03:19, Bin.Cheng wrote:
> On Tue, Nov 24, 2015 at 6:18 PM, Richard Earnshaw
> <Richard.Earnshaw@foss.arm.com> wrote:
>> On 24/11/15 09:56, Richard Earnshaw wrote:
>>> On 24/11/15 02:51, Bin.Cheng wrote:
>>>>>> The aarch64's problem is we don't define addptr3 pattern, and we don't
>>>>>>>> have direct insn pattern describing the "x + y << z".  According to
>>>>>>>> gcc internal:
>>>>>>>>
>>>>>>>> ‘addptrm3’
>>>>>>>> Like addm3 but is guaranteed to only be used for address calculations.
>>>>>>>> The expanded code is not allowed to clobber the condition code. It
>>>>>>>> only needs to be defined if addm3 sets the condition code.
>>>>>>
>>>>>> addm3 on aarch64 does not set the condition codes, so by this rule we
>>>>>> shouldn't need to define this pattern.
>>>> Hi Richard,
>>>> I think that rule has a prerequisite that backend needs to support
>>>> register shifted addition in addm3 pattern.
>>>
>>> addm3 is a named pattern and its format is well defined.  It does not
>>> take a shifted operand and never has.
>>>
>>>> Apparently for AArch64,
>>>> addm3 only supports "reg+reg" or "reg+imm".  Also we don't really
>>>> "does not set the condition codes" actually, because both
>>>> "adds_shift_imm_*" and "adds_mul_imm_*" do set the condition flags.
>>>
>>> You appear to be confusing named patterns (used by expand) with
>>> recognizers.  Anyway, we have
>>>
>>> (define_insn "*add_<shift>_<mode>"
>>>   [(set (match_operand:GPI 0 "register_operand" "=r")
>>>         (plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r")
>>>                               (match_operand:QI 2
>>> "aarch64_shift_imm_<mode>" "n"))
>>>                   (match_operand:GPI 3 "register_operand" "r")))]
>>>
>>> Which is a non-flag setting add with shifted operand.
>>>
>>>> Either way I think it is another backend issue, so do you approve that
>>>> I commit this patch now?
>>>
>>> Not yet.  I think there's something fundamental amiss here.
>>>
>>> BTW, it looks to me as though addptr<m>3 should have exactly the same
>>> operand rules as add<m>3 (documentation reads "like add<m>3"), so a
>>> shifted operand shouldn't be supported there either.  If that isn't the
>>> case then that should be clearly called out in the documentation.
>>>
>>> R.
>>>
>>
>> PS.
>>
>> I presume you are aware of the canonicalization rules for add?  That is,
>> for a shift-and-add operation, the shift operand must appear first.  Ie.
>>
>> (plus (shift (op, op)), op)
>>
>> not
>>
>> (plus (op, (shift (op, op))
> 
> Hi Richard,
> Thanks for the comments.  I realized that the not-recognized insn
> issue is because the original patch build non-canonical expressions.
> When reloading address expression, LRA generates non-canonical
> register scaled insn, which can't be recognized by aarch64 backend.
> 
> Here is the updated patch using canonical form pattern,  it passes
> bootstrap and regression test.  Well, the ivo failure still exists,
> but it analyzed in the original message.
> 
> Is this patch OK?
> 
> As for Jiong's concern about the additional extension instruction, I
> think this only stands for atmoic load store instructions.  For
> general load store, AArch64 supports zext/sext in register scaling
> addressing mode, the additional instruction can be forward propagated
> into memory reference.  The problem for atomic load store is AArch64
> only supports direct register addressing mode.  After LRA reloads
> address expression out of memory reference, there is no combine/fwprop
> optimizer to merge instructions.  The problem is atomic_store's
> predicate doesn't match its constraint.   The predicate used for
> atomic_store<mode> is memory_operand, while all other atomic patterns
> use aarch64_sync_memory_operand.  I think this might be a typo.  With
> this change, expand will not generate addressing mode requiring reload
> anymore.  I will test another patch fixing this.
> 
> Thanks,
> bin

Some comments inline.

>>
>> R.
>>
>> aarch64_legitimize_addr-20151128.txt
>>
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index 3fe2f0f..5b3e3c4 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -4757,13 +4757,65 @@ aarch64_legitimize_address (rtx x, rtx /* orig_x  */, machine_mode mode)
>>       We try to pick as large a range for the offset as possible to
>>       maximize the chance of a CSE.  However, for aligned addresses
>>       we limit the range to 4k so that structures with different sized
>> -     elements are likely to use the same base.  */
>> +     elements are likely to use the same base.  We need to be careful
>> +     not split CONST for some forms address expressions, otherwise it

not to split a CONST for some forms of address expression,

>> +     will generate sub-optimal code.  */
>>  
>>    if (GET_CODE (x) == PLUS && CONST_INT_P (XEXP (x, 1)))
>>      {
>>        HOST_WIDE_INT offset = INTVAL (XEXP (x, 1));
>>        HOST_WIDE_INT base_offset;
>>  
>> +      if (GET_CODE (XEXP (x, 0)) == PLUS)
>> +	{
>> +	  rtx op0 = XEXP (XEXP (x, 0), 0);
>> +	  rtx op1 = XEXP (XEXP (x, 0), 1);
>> +
>> +	  /* For addr expression in the form like "r1 + r2 + 0x3ffc".
>> +	     Since the offset is within range supported by addressing
>> +	     mode "reg+offset", we don't split the const and legalize
>> +	     it into below insn and expr sequence:
>> +	       r3 = r1 + r2;
>> +	       "r3 + 0x3ffc".  */

I think this comment would read better as

	/* Address expressions of the form Ra + Rb + CONST.

	   If CONST is within the range supported by the addressing
	   mode "reg+offset", do not split CONST and use the
	   sequence
		Rt = Ra + Rb
		addr = Rt + CONST.  */

>> +	  if (REG_P (op0) && REG_P (op1))
>> +	    {
>> +	      machine_mode addr_mode = GET_MODE (x);
>> +	      rtx base = gen_reg_rtx (addr_mode);
>> +	      rtx addr = plus_constant (addr_mode, base, offset);
>> +
>> +	      if (aarch64_legitimate_address_hook_p (mode, addr, false))
>> +		{
>> +		  emit_insn (gen_adddi3 (base, op0, op1));
>> +		  return addr;
>> +		}
>> +	    }
>> +	  /* For addr expression in the form like "r1 + r2<<2 + 0x3ffc".
>> +	     Live above, we don't split the const and legalize it into
>> +	     below insn and expr sequence:

Similarly.
>> +	       r3 = 0x3ffc;
>> +	       r4 = r1 + r3;
>> +	       "r4 + r2<<2".  */

Why don't we generate

  r3 = r1 + r2 << 2
  r4 = r3 + 0x3ffc

utilizing the shift-and-add instructions?

>> +	  else if (REG_P (op0) || REG_P (op1))
>> +	    {
>> +	      machine_mode addr_mode = GET_MODE (x);
>> +	      rtx base = gen_reg_rtx (addr_mode);
>> +
>> +	      /* Switch to make sure that register is in op0.  */
>> +	      if (REG_P (op1))
>> +		std::swap (op0, op1);
>> +
>> +	      rtx addr = gen_rtx_fmt_ee (PLUS, addr_mode, op1, base);
Use gen_rtx_PLUS.

>> +
>> +	      if (aarch64_legitimate_address_hook_p (mode, addr, false))
>> +		{
>> +		  base = force_operand (plus_constant (addr_mode,
>> +						       op0, offset),
>> +					NULL_RTX);
>> +		  return gen_rtx_fmt_ee (PLUS, addr_mode, op1, base);
Likewise.

>> +		}
>> +	    }
>> +	}
>> +
>>        /* Does it look like we'll need a load/store-pair operation?  */
>>        if (GET_MODE_SIZE (mode) > 16
>>  	  || mode == TImode)

R.