Page MenuHomePhabricator

[AArch64]: Add support for parsing rN registers.
ClosedPublic

Authored by manojgupta on Mar 22 2018, 9:04 PM.

Details

Summary

Allow rN registers to be simply parsed as correspoing xN registers.
The "register ... asm("rN")" is an command to the
compiler's register allocator, not an operand to any individual assembly
instruction. GCC documents this syntax as "...the name of the register
that should be used."

This is needed to support the changes in Linux kernel (see
https://lkml.org/lkml/2018/3/1/268 )

Note: This will add support only for the limited use case of
register ... asm("rN"). Any other uses that make rN leak into assembly
are not supported.

Diff Detail

Repository
rL LLVM

Event Timeline

manojgupta created this revision.Mar 22 2018, 9:04 PM

Indenting and a minor fix.

Add more context from the email communication with Peter Smith and Robin Murphy.

On 22/03/18 02:34, Manoj Gupta wrote:

Hi Robin,

this is Manoj from Google. I was looking into implementing support for
parsing "r" registers in clang for AArch64 based on lkml thread
https://lkml.org/lkml/2018/3/1/186 ).

On the llvm bug(https://bugs.llvm.org/show_bug.cgi?id=36862#c2 ), Peter had
a comment that I think could be better clarified by you.

"
Can you be a bit more specific about where in GCC "rn" is acceptable? It is
certainly not true in the general case, for example:
...
"
Further, my understanding is that binutils does not support "r" registers
in AArch64.
So is it correct that GCC support limited to parsing "r" registers as "x"
registers and assembler will never see "r" registers?

There are two distinct things being conflated here: "r0" is not, and
never has been, a valid A64 assembly *operand*, thus indeed should never
be passed to AArch64 binutils since the latter only consumes A64
assembly syntax.

The "register ... asm("r0")" case, however, is an command to the
compiler's register allocator, not an operand to any individual assembly
instruction. GCC documents this syntax as "...the name of the register
that should be used.", and section B1.2.1 of the Armv8 ARM[1] is quite
clear that the fundamental AArch64 registers are named R0-R30, SP, PC,
and V0-V31. It seems perfectly straightforward to me that the register
allocator would work in units of fundamental machine registers, while
access/element sizes only start to matter in the subsequent assembly
generation phase where actual instruction accesses to those registers
are emitted (and indeed where those sizes depend more on each specific
operation than the inherent size of the type being operated on).

I note that Clang *does* already understand references to SIMD&FP "V"
registers in this context as expected[2] without demanding an explicit
access or element size.

Robin.

[1]
https://developer.arm.com/products/architecture/a-profile/docs/ddi0487/latest/arm-architecture-reference-manual-armv8-for-armv8-a-architecture-profile
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/567872.html

Peter also requested that a test be added to make sure that rY was not accepted by the Clang assembler as a true synonym for xY.

lib/Basic/Targets/AArch64.cpp
320 ↗(On Diff #139556)

For x29, x30, you should really be grouping them together. For instance:

{{"r29", "fp"}, "x29}, {{"r30", "lr"}, "x30"}

This lets you remove the x29/fp and x30/lr from the first line.

Peter also requested that a test be added to make sure that rY was not accepted by the Clang assembler as a true synonym for xY.

Yes, I am working on that. Wanted to send this out first.

  1. Added test case to verify register names are not renamed in unsupported cases.
  2. Updated formatting.

I think that we may have a bit of a conceptual difference with GCC here. As far as I can tell in GCC it doesn't matter if "rn", "wn", "xn" or even "bn" is used, this refers to register 0. The Operand constraint such as %w0 is used to control the substitution so the r registers aren't strictly aliases like the other registers. The comment

The S/D/Q and W/X registers overlap, but aren't really aliases; we
don't want to substitute one of these for a different-sized one.

suggests that this may have been the intent that Clang behave differently to GCC in this respect. For example in Clang there appears to be a bit more significance placed on the "wn" or "xn", for example clang will warn in the following example, whereas gcc will not:

long f2(int i) {
    register int x asm("w6");
    register int y asm("w7");
    register int z asm("w8");
    asm("add %0, %1, %2\n": "=r"(x) : "r"(y), "r"(z));
    return y;
}

t2.c:5:34: warning: value size does not match register size specified by the
      constraint and modifier [-Wasm-operand-widths]
    asm("add %0, %1, %2\n": "=r"(x) : "r"(y), "r"(z));
                     ^~
                     %w0
// and so on for %1 and %2.

Having said all that, clang will not warn when the operand modifier is explicit, for example:

long f2(int i) {
    register long x asm("w6");
    register long y asm("w7");
    register long z asm("w8");
    asm("add %w0, %w1, %w2\n": "=r"(x) : "r"(y), "r"(z));
    return y;
}

At the moment I'm not too sure what to make of this. Strictly speaking I think "rn" is an unsized register that Clang shouldn't warn about size mismatch in the operand modifier. With the implementation above it won't because we default to xn and clang doesn't seem to warn when there is an explicit modifier for wn, although this could change in the future.

I'd be very interested to hear other opinions on this? If register rn is to be supported perhaps a test that clang doesn't warn when asm("rn") is used with the operand modifier %wn is used.

The warning you're seeing is because without an operand modifier Clang always chooses an x-register in its textual assembly expansion. This 64-bit default is compared to the underlying C type to determine whether a warning should be emitted, and I think that's reasonable. The C type is all that's available for most uses (without a register keyword) and it seems consistent to use it even when register has been specified on the variable.

Where we might want to introduce a new warning is for a mismatch like:

register uint64_t var asm("w0"); // 64-bits in a 32-bit register. Magic!

But that seems pretty separate to this patch really.

But

Thanks for the clarification. With that in mind I'm much less concerned that adding "r" as an alias will make extra warnings more difficult. I agree that there should be at least a warning for register long x asm ("wn") although that is separate from this patch. Has anyone else got any objections?

peter.smith accepted this revision.Mar 29 2018, 3:04 AM

Given that this is important for compiling the Linux kernel with clang I think that it is worth improving compatibility with GCC. LGTM.

This revision is now accepted and ready to land.Mar 29 2018, 3:04 AM
This revision was automatically updated to reflect the committed changes.