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

[Bin.Cheng]

On Tue, Nov 24, 2015 at 5:56 PM, Richard Earnshaw
<Richard.Earnshaw@foss.arm.com> 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
I understand the difference between expanding and recognizer.  I
missed below non-set-condition-flags insn patterns as you pointed out,
that's what I meant we don't support "does not set the condition
codes" wrto either expanding or recognizer.  Anyway, I was wrong and
we do have such nameless patterns.

>
> (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.
The direct cause of this ICE is LRA generates below insn sequence when
reloading "x=y+z*const" where the mult part is actually a register
scale operation:
    r1 = z*const
    x = y + r1
It generates mult directly but aarch64 doesn't have pattern describing
mult with constant.  We do have pattern for the corresponding shift
insn.  As a result, the constant mult insn is not recognized.  This
can be fixed by using force_operand to generate valid mult or shift
instructions, is it feasible in lra?
For this case, we can do better reload if we support scaled add in
addptr pattern.  After some
archaeological work, I found addptr is added for s390 which supports
addressing mode like "base + index + disp" and its variants.  Maybe
that's why the pattern's documentation doesn't explicitly describe the
scaled operation.  IMHO, this pattern should be target dependent and
be able to handle addressing modes that each target supports.  A side
proof is in code and comment of function lra_emit_add.  It explicitly
handles, describes scaled add address expression.

CCing Vlad and Andreas since they are the original authors and may help here.

Thanks,
bin