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

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

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

R.