Incorrect code generation for ARM with inline assembly

[jabraham17]

Bugzilla Link	50647
Resolution	INVALID
Resolved on	Jul 13, 2021 08:48
Version	unspecified
OS	All
CC	@DavidSpickett,@zygoloid

Extended Description

I came across a code generation issue related to register allocation.

Essentially, if you have two C variables, one being uninitialized, and you use one of them in regular C code (in the example a shift right), then use both of them as inputs to an inline asm block, the two variables collide on a register. This issue does not appear in similar X86 code or in the ARM gcc compiler. However, if you initialize the variable this bug goes away, but introduces an extra mov that is unnecessary for the correctness of the code.

You can see the below example on compiler explorer here (https://godbolt.org/z/7nKhfj9E9)

The following is code that reproduces the issue
void func(unsigned long long n)
{
#ifndef FIX
unsigned long long b;
#else
//this fixes the bug but introduces an extra, extraneous mov
//the ideal code would not include this extra mov
//this is what gcc does with or without initializing the variable
unsigned long long b = 0;
#endif

//if this is not here, bug does not occur
//one of the C variables must be used to trigger the bug
n = n >> 7;

//_n should be a loop counter
//_b should be the jump address
//but _n and _b map to the same register in arm clang
//they should map to different registers, as seen in arm gcc
__asm__ volatile (
    "top%=: \n\t"
    "adrp %[_b], lbl%=@PAGE \n\t"
    "add %[_b], %[_b], lbl%=@PAGEOFF \n\t"
    "br %[_b] \n\t"
    "lbl%=: \n\t"
    "sub %[_n], %[_n], #​0x1 \n\t"
    "cmp %[_n], #​0x0 \n\t"
    "b.ne top%= \n\t"
    : 
    : [_n] "r" (n), [_b] "r" (b)
);

}

The generated assembly is as follows
func:
lsr x8, x0, #​7
top0:
adrp x8, lbl0@PAGE
add x8, x8, lbl0@PAGEOFF
br x8
lbl0:
sub x8, x8, #​1
cmp x8, #​0
b.ne top0
ret

Note that x0 is 'n', which after being shifted is stored in x8. But then x8 is used for both 'n' and 'b'.

I would expect the generated assembly to be
func:
lsr x9, x0, #​7
top0:
adrp x8, lbl0@PAGE
add x8, x8, lbl0@PAGEOFF
br x8
lbl0:
sub x9, x9, #​1
cmp x9, #​0
b.ne top0
ret

[DavidSpickett]

I was able to reproduce this with a modified example for riscv/x86/arm and aarch64.

https://godbolt.org/z/zTx3Ks7oK

Does that clash with what you've seen with x86?

If this is deliberate it may be due to the value being undef and only an input. Though I agree using the same register is surprising.

I did try adding the undef var as an output to see if that would tip the balance but apparently not, and it just got more confusing.

Let me know if you see something different with your x86 code (there may be some aspect I'm missing) and I'll see if I can find where the decision is made.

[jabraham17]

So this does clash with what I saw with x86. Here is a modified version of the original example with the equivalent x86 code (https://godbolt.org/z/b4dKcEs6f). You can see that the x86 code generates correctly using two separate registers.

Your modified example is interesting in that GCC still gets it right but now x86 is also incorrect. I would also note that I played with it a bit, and if you comment out the "n = 99" the x86 and aarch64 clang are now correct, but armv7 and riscv clang are still wrong. (https://godbolt.org/z/G1qb75qno).

[DavidSpickett]

I'm still looking at this, clearly this is something to do with register allocation when a value is undef. As you can see in the IR:
%2 = tail call i64 asm sideeffect "add $2, $2, $2 \0A\09add $1, $1, $1 \0A\09", "=r,r,r"(i64 %0, i64 undef) #​2, !dbg !​22, !srcloc !​23

I think the logic goes something like "if the value is undef, there's no reason to give it a unique register".

What I haven't found yet, is the place that encodes that logic. It may be that for inline asm we can justify an exception but without knowing the reasoning fully I can't say.

It's somewhere in VirtRegRewriter, that's as far as I've got.

As for the n=99 I believe what happens here is that by saying n=99 you effectively delete whatever the initial argument value of n was.

Without n=99, n will be argument register 0, so we re-use that register for the value of n. Then it seems each target has it's own logic for assigning temporary registers for b.

x86 and AArch64 start with non argument registers (which may just be an implementation artifact, though it does make sense) and armv7 and riscv just start at the first register.

With n=99, there's no reason to reference the value that would be in argument register 0. So we assign a new temp register to n, then we allow the undef (b) to occupy the same register.

[jabraham17]

This makes sense to me. I would add one thing as well. When you set n=99, there is no need for the argument register and so 99 gets put in a temporary register, as you said. However if you do something like n=n+99, where you still use n, this still gets put in a temporary register although to the view of the programmer the value of n is "gone". It would make logical sense to overwrite the value since you only want to use the updated one.

[DavidSpickett]

Me finding the place this happens is somewhat orthogonal to whether it should be fixed to match gcc, so I've posted on the cfe-dev list to get opinions on that:
https://lists.llvm.org/pipermail/cfe-dev/2021-July/068529.html

Feel free to chime in there yourself.

[DavidSpickett]

So I got some good feedback from the list and this is the summary.

An undef value can be considered to have any value the compiler chooses.
So clang chooses the value to be the same as the other input, so you get the
same register allocated to it.

It could choose that the undef is != the other value and maybe this is what gcc does. But both are valid choices given that definition of undef.

You can also see the same registers allocated if you have two inputs of the same (defined) value. Both gcc and clang will give you the same register.

So for your code as reported this is working as expected and we won't be changing anything.

However, someone else pointed out that there's no clobbers for the inline asm statement.
If you use "+r" you can mark them as inputs and outputs and you'll get unique registers. Like:
asm volatile (
"add %[_b], %[_b], %[_b] \n\t"
"add %[_n], %[_n], %[_n] \n\t"
: [_n] "+r" (n), [_b] "+r" (b)
);

"Operands using the ‘+’ constraint modifier count as two operands (that is, both as input and output) towards the total maximum of 30 operands per asm statement."
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

That could mean you see register restore instructions after the asm statement at low optimisation levels but I'd expect them to be removed from -O1 onwards.

And potentially you could have issues if this function was inlined using LTO, even if at the source level the asm is at the end of a function returning void. (just a guess though)

[jabraham17]

Thank you for looking into this. The reasoning on undef values somewhat makes sense to me, but regardless the "+r" gives the desired effect in all of my example cases.

[DavidSpickett]

No problem, was an interesting issue.

And potentially you could have issues if this function was inlined using LTO, even if at the source level the asm is at the end of a function returning void. (just a guess though)

I should clarify I meant "if you don't mark the clobbers in some way you could have issues...".

Resolving as invalid aka working as intended.

[DavidSpickett]

mentioned in issue llvm/llvm-bugzilla-archive#50718