AArch64Address.lea missed BASE_REGISTER_ONLY case

[Dmitry Samersoff]

Andrew,

> So, if you want to push this point and try to make a case for
> your change going in I suggest you ask one of the Graal reviewers to
> consider it.

Thank you for your time!

I better understand now how things work and will not push for the
changes I proposed.

It might be helpful to add a summary of this topic as comments to
AArch64Address.lea.

-Dmitry

On 01.03.2018 14:12, Andrew Dinn wrote:
> On 28/02/18 17:52, Dmitry Samersoff wrote:
>> Thank you for a detailed explanation. I understand the rationally behind
>> this error, but couldn't say that API is consistent.
> 
> Let's deal with your proposed change first.
> 
>> 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;
> 
> Do you agree that providing two ways to implement a register to register
> move is a bad thing? The code base will certainly not be clearer or
> easier to maintain if we support this as an alternative way to schedule
> a move operation. If you acknowledge that then we are one step forward
> and we can infer that your proposed fix is the wrong way to go. If not
> do you have a use case which justifies allowing BASE_REGISTER addresses
> to be passed in to lea?
> 
> Let's move on to your four use cases
> 
>> Four constructions below do essentially the same but (a) and (b)
>> works, when (c) and (d) cause an assert
>   . . .
>>
>> 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));
> Yes, a and b work and c and d get caught. But the obvious comment to
> make before investigating why the first two are not rejected is that
> none of these four examples as you present them make any sense. In all
> cases you are simply doing a move in one of four pointlessly convoluted
> ways.
> 
> Ok, so why do a and b work? It might look like it is worth adding checks
> to reject the first two cases should they ever arise but I don't think
> that is appropriate. What is needed is just not to code dumb sequences
> like these. However, less dumb variants need to pass.
> 
> Case a is not rejected by the lea implementation because it is possible
> that code which generates immediate addresses might pass a computed
> value rather than a literal argument and not easily be able to special
> the case where that computed value is zero. This can happen legitimately
> even with very simple computed values. For example, when unrolling a
> loop you might be loading effective addresses for a sequence of array
> entries relative to a base address and use offsets i = 0, i = 4, i = 8
> and i = 12 (or ). Calling lea with i as the offset should be allowed
> even if in the first call i is bound to 0.
> 
>  Of course, with register offset addressing it can always be the case
> that the offset register contains zero.  However, case b as you present
> it is a truly dumb idiom (the obvious thing to write would be case 3
> which fails). It still passes because of related cases which do make
> sense. The same combined sequence might happen in two independent steps
> because the copy from zr into r occurs in some separate part of the code
> generator to the lea call. It would be nice to be able to avoid such
> cases but that is not always going to be straightforward. If it happens
> in successive lines of code as you present it then it is easy to fix but
> note: the problem in this sort of example lies not in the api but in the
> badly written client code.
> 
> Now, strictly, that is all just my opinion. I am not a reviewer for the
> Graal project (although I am a reviewer for the jdk development
> project). So, if you want to push this point and try to make a case for
> your change going in I suggest you ask one of the Graal reviewers to
> consider it.
> 
> regards,
> 
> 
> Andrew Dinn
> -----------
> 


-- 
Dmitry Samersoff
http://devnull.samersoff.net
* There will come soft rains ...