[aarch64-port-dev ] Notations and reliability

[Andrew Haley]

On 28/03/17 11:28, Andrew Dinn wrote:
> On 28/03/17 10:16, Andrew Haley wrote:
>> We've come a cropper a couple of times because of poorly-chosen
>> notations.  Several times we've had a confusion between iRegI and
>> iRegINoSp, which causes very obscure crashes.  Also, a little while
>> ago I found several bugs (all mine) caused by getting mixed up between
>> e.g. strw and str.
>>
>> I now believe that I made a mistake when I assumed that Xwords
>> (i.e. 64 bits) were in some way the "default" size on AArch64 and
>> omitted the X suffix on them.  In hindsight, it is much better for
>> every operation to have a size associated with it, so the programmer
>> would be forced to choose which one to use, and state it explicitly.
>> This is how the "standard" assembly language works, and it is better
>> than what we do.
>>
>> [Interestingly, the Oracle-authored AArch64 port made exactly the
>> same (poor, IMO) choice.  Perhaps programmers' brains are isomorphic.]
>>
>> With regard to register classes, GCC uses "GENERAL_REGS" as the name
>> of the register class that can be allocated as a general-purpose
>> operands, with "ALL_REGS" for everything.  It would be better we did
>> something similar.
> 
> We do actually have quite clear names for the register /classes/ much as
> per x86. The names giving rise to the confusion are those employed in
> the /operand/ definitions. Note that unlike the register classes these
> all come in multiple flavours -- I, L,P, N etc -- according to the type
> of value that will be found in the register. I don't think our names are
> actually that much more opaque than x86. For example, it has
> 
>   rRegI
>   rRegP
>   any_RegP
>   no_rax_rdx_RegL
>   ...
> 
>   It's not actually very clear what is the difference between rRegP and
> any_RegP until you refer back to the register classes.

Perhaps, but being no worse than x86 is a low bar.  We should be able
to improve on that.

> iRegX matches any register holding a value of type X from R0 to R31.
> iRegXNoSp matches any non-special register holding a value of type X
> from R0 to R26 i.e. it omits the special registers R27 to R31 that are
> dedicated to storing VM/frame state i.e. rheapbase, rthread, fp, lr and sp.
> 
> We could perhaps relabel these to
> 
>   iRegXAll and iRegXGen
> 
> or some other name of choice. However, I am not sure that it is any more
> apparent to anyone who doesn't already understand what is going on that
> an All (or, say, Any) register is any more or less general than a Gen
> register.

All else being equal, people tend to prefer short names.  I was
thinking of something along the lines of RegI, RegL etc. to replace
the iRegXNoSP, and AnyRegX to replace iRegX.  (That "i" prefix is
awkward and pointless on this architecture, so I think we won't miss
it.  We don't put floats into integer registers.)  RegX is almost
always harmless, I think, except that we need to be able to move to
and from special registers.

> I think the real problem here is that those of us who do know
> what is going on did not adequately review the proposed changes which
> mistakenly employed the wrong register types.

It's both.  The notation itself is error-prone, and the reviewer (me,
probably) didn't spot the errors.

The key here is to make the odd case stand out.  It'd be in red if
I had my way.

> Will this change help us? I am not really convinced. With any of these
> current or possible future names it is very apparent what registers any
> given operand will select for matching or assignment if i) you know/look
> up the register classes they are derived from and ii) you understand how
> that definition gets used by the matcher/register allocator.
> 
> The 3rd operand iRegIorL2I is specific to AArch64 rules.

Sure, I don't think we need to change that one, except for the "i".

> I don't think this is a particularly bad name and would not really want
> to change it. More importantly, as before, I don't think any name is
> going to help someone who doesn't already understand what is going
> recognise what this rule does and know how to use this operand.
> 
> If we do anything I would prefer just to expand iRegXNoSp to
> iRegXNoSpecial which avoids the potential ambiguous reading that it
> means no Stack Pointer. That may invite 'lay' readers to wonder what
> NoSpecial means and perhaps, in consequence, to consider what they are
> doing and ask for advice before trying to write new rules.

I think that makes it worse, and invite you to consider the
possibility that you're a bit too close to this to see it from the POV
of the maintenance programmer.  Wording it in the negative in this way
is rather unhelpful because it looks like an exception, whereas in fact
it is an allocatable register.  The "safe" default should be the easy
short one.

Andrew.