AArch64Address.lea missed BASE_REGISTER_ONLY case

[Gilles Duboscq]

Hi,

I think both options have their merit.
Asserting for this kind of pattern is useful in early development to detect sub-optimal patterns.
However going with that logic, something like `AArch64Address.createUnscaledImmediateAddress(array1, 0))` should also assert and force you to use `AArch64Address.createBaseRegisterOnlyAddress(array1))`.

In the end, I think it's more valuable to be robust and deal with sub-optimal patterns in an other way.
These `lea` method seem a bit strange: they accept an address but choke on most `AddressingModes`.

Also, if you look at the AMD64 or SPARC addresses, instead of having an `AddressingMode` field, the addressing mode is derived from the state of the base, index, scale etc. fields.

 Gilles


On 28/02/18 18:52, Dmitry Samersoff wrote:
> Hi Andrew,
> 
> Thank you for a detailed explanation. I understand the rationally behind
> this error, but couldn't say that API is consistent.
> 
> Four constructions below do essentially the same but (a) and (b) works,
> when (c) and (d) cause an assert.
> 
> It might be better to handle BASE_REGISTER_ONLY the same way as
> IMMEDIATE_UNSCALED i.e.:
> 
>  case BASE_REGISTER_ONLY:
>    if (base.equals(r)) { // it's a nop
>       break;
>    }
> 
>    masm.mov(64, r, base);
>    break;
> 
> 
> a)
> masm.lea(reg,
>       AArch64Address.createUnscaledImmediateAddress(array1, 0));
> 
> b)
> masm.mov(64, tmp, zr);
> masm.lea(reg,
>     AArch64Address.createRegisterOffsetAddress(array1, tmp, false));
> 
> c)
> masm.lea(reg,
>     AArch64Address.createRegisterOffsetAddress(array1, zr, false));
> d)
> masm.lea(reg,
>      AArch64Address.createBaseRegisterOnlyAddress(array1));
> 
> -Dmitry
> 
> On 02/28/2018 01:21 PM, Andrew Dinn wrote:
>> Hi Dimitry,
>>
>> On 27/02/18 18:45, Dmitry Samersoff wrote:
>>> Is it a bug? If yes, I'll create a pull requires with the fix.
>> Sorry, but no. lea means load effective address. It is there to compute
>> an address from a base address provided in some given register combined
>> with an offset. The latter is provided either via another register or as
>> a constant which can be encoded as an immediate. Essentially, it does an
>> add.
>>
>> If you just have a base register with no offset to add then lea would
>> simply reduce to a register copy. In which case . . . this is not the
>> instruction you were looking for. Throwing a Graal shouldNotReachHere
>> error is the correct response.
>>
>> At present the only place where AArch64ddress.lea gets called in from
>> MacroAssembler.lea and that only ever gets called from
>> AArch64ArrayEqualsOp with either a register offset address or an
>> unscaled immediate address. So, all is well.
>>
>> regards,
>>
>>
>> Andrew Dinn
>> -----------
>> Senior Principal Software Engineer
>> Red Hat UK Ltd
>> Registered in England and Wales under Company Registration No. 03798903
>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>>
> 
>