AArch64Address.lea missed BASE_REGISTER_ONLY case

[Andrew Dinn]

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