Re: [PATCH]Construct canonical scaled address expression in IVOPT

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Sep 24, 2013 at 8:20 AM, bin.cheng <bin.cheng@arm.com> wrote:
>
>
>> -----Original Message-----
>> From: Richard Biener [mailto:richard.guenther@gmail.com]
>> Sent: Monday, September 23, 2013 8:08 PM
>> To: Bin Cheng
>> Cc: GCC Patches
>> Subject: Re: [PATCH]Construct canonical scaled address expression in IVOPT
>>
>> On Fri, Sep 20, 2013 at 12:00 PM, bin.cheng <bin.cheng@arm.com> wrote:
>> > Hi,
>> > For now IVOPT constructs scaled address expression in the form of
>> > "scaled*index" and checks whether backend supports it. The problem is
>> > the address expression is invalid on ARM, causing scaled expression
>> > disabled in IVOPT on ARM.  This patch fixes the IVOPT part by
>> > constructing rtl address expression like "index*scaled+base".
>>
>> -      addr = gen_rtx_fmt_ee (MULT, address_mode, reg1, NULL_RTX);
>> +      /* Construct address expression in the canonical form of
>> + "base+index*scale" and call memory_address_addr_space_p to see
>> whether
>> + it is allowed by target processors.  */
>> +      index = gen_rtx_fmt_ee (MULT, address_mode, reg1, NULL_RTX);
>>        for (i = -MAX_RATIO; i <= MAX_RATIO; i++)
>>   {
>> -  XEXP (addr, 1) = gen_int_mode (i, address_mode);
>> +  if (i == 1)
>> +    continue;
>> +
>> +  XEXP (index, 1) = gen_int_mode (i, address_mode);  addr =
>> + gen_rtx_fmt_ee (PLUS, address_mode, index, reg1);
>>    if (memory_address_addr_space_p (mode, addr, as))
>>      bitmap_set_bit (valid_mult, i + MAX_RATIO);
>>
>> so you now build reg1*scale+reg1 - which checks if offset and scale work
> at
>> the same time (and with the same value - given this is really
> reg1*(scale+1)).
>> It might be that this was implicitely assumed (or that no targets allow
> scale
>> but no offset), but it's a change that requires audit of behavior on more
>> targets.
> So literally, function multiplier_allowed_in_address_p should check whether
> multiplier is allowed in any kind of addressing mode, like [reg*scale +
> offset] and [reg*scale + reg].

Or even [reg*scale] (not sure about that).  But yes, at least reg*scale + offset
and reg*scale + reg.

>   Apparently it's infeasible to check every
> possibility for each architecture, is it ok we at least check "index", then
> "addr" if "index" is failed?  By "any kind of addressing modes", I mean
> modes supported by function get_address_cost,  i.e., in form of "[base] +
> [off] + [var] + (reg|reg*scale)".

I suppose so, but it of course depends on what IVOPTs uses the answer
for in the end.  Appearantly it doesn't distinguish between the various cases
even though TARGET_MEM_REF does support all the variants in question
(reg * scale, reg * scale + reg, reg * scale + const, reg * scale +
reg + const).

So the better answer may be to teach the costs about the differences?

>> The above also builds more RTX waste which you can fix by re-using the
> PLUS
>> by building it up-front similar to the multiplication.  You also miss the
> Yes, this can be fixed.
>
>> opportunity to have scale == 1 denote as to whether reg1 + reg2 is valid.
> I
>> would expect that many targets support reg1 * scale + constant-offset but
>> not many reg1 * scale + reg2.
> I thought scale==1 is unnecessary because the addressing mode degrades into
> "reg" or "reg+reg".  Moreover, calls of multiplier_allowed_in_address_p in
> both get_address_cost and get_computation_cost_at have scale other than 1.

Ok.

>>
>> So no, the helper now checks sth completely different.  What's the problem
>> with arm supporting reg1 * scale?  Why shouldn't it being able to handle
> the
>> implicit zero offset?
>
> As Richard clarified, ARM does not support scaled addressing mode without
> base register.

I see.

Richard.