Page MenuHomePhabricator

[AArch64] Do not emit '#' before immediates in inline asm
ClosedPublic

Authored by pirama on Jul 31 2019, 5:44 PM.

Details

Summary

The A64 assembly language does not require the '#' character to
introduce constant immediate operands. Avoid the '#' since the AArch64
asm parser does not accept '#' before the lane specifier and rejects the
following:

__asm__ ("fmla v2.4s, v0.4s, v1.s[%0]" :: "I"(0x1))

Fix a test to not expect the '#' and add a new test case with the above
asm.

Fixes: https://github.com/android-ndk/ndk/issues/1036

Diff Detail

Repository
rL LLVM

Event Timeline

pirama created this revision.Jul 31 2019, 5:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 5:44 PM

I think that this will need a wider review to make check if anyone is dependent on the '#'. My personal opinion is that this is a good change to make as it will make the 'I' constraint more generally useful, without it we may need a new constraint that doesn't add a '#':

  • The # is optional for immediates.
  • GNU assembler and Clang both fault # in an element index, correctly according to my reading of the Arm ARM.
  • GCC does not output the # when expanding the 'I' constraint
    • A colleague mentioned that there may be other cases where a # might be problematic, such as ".byte %0" in a switched section.

Some references:
https://static.docs.arm.com/ddi0487/ea/DDI0487E_a_armv8_arm.pdf

C1.2.1 General requirements
...
The A64 assembly language does not require the # character to introduce constant immediate operands, but an assembler must allow immediate values introduced with or without the # character. Arm recommends that an A64 disassembler outputs a # before an immediate operand.

In the Arm ARM immediates are described as #<imm> with the # character in the assembly language description, the element index is described differently without the #, for example: <Vm>.<Ts>[<index>].

A colleague has pointed out to me that there is a way of using the c modifier after the % "Require a constant operand and print the constant expression with no punctuation" that works with both clang and GCC. See https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#InputOperands in your example above it would be:

__asm__ ("fmla v2.4s, v0.4s, v1.s[%c0]" :: "I"(0x1))

I still think that it could be useful to have GCC and Clang have the same behaviour for "I", but at least there is a workaround available for this case.

Thanks for the workaround @peter.smith. I suggested it to the NDK developer as well. The immediate need for this change is gone, but I agree that this is useful for the various reasons you outlined.

This only affects inline asm, it looks like, not general instructions? (I looked briefly, and it looks like instructions go through AArch64InstPrinter::printOperand instead.)

I don't think this would cause any issues off the top of my head, but you might want to run something like an Android build to confirm that.

pirama added a comment.Aug 5 2019, 3:32 PM

This only affects inline asm, it looks like, not general instructions? (I looked briefly, and it looks like instructions go through AArch64InstPrinter::printOperand instead.)

I don't think this would cause any issues off the top of my head, but you might want to run something like an Android build to confirm that.

I built Android with this change and except for one issue [1], the build is successful.

[1] a python script that had the '#' in a regular expression when searching the emitted inline assembly.

peter.smith accepted this revision.Aug 6 2019, 2:14 AM

I've built a defconfig aarch64 linux kernel without errors including this change.

I think that if there is going to be a problem it will be in scripts like the aforementioned python script that check the output of clang -S. I think that is small enough a change to correct quite easily. I've set approved as this looks good to me. It should also be reversible quite easily if it does cause any unforeseen problems.

This revision is now accepted and ready to land.Aug 6 2019, 2:14 AM
pirama added a comment.Aug 6 2019, 9:16 AM

Thanks Peter. I'll wait for another day for further comments, and if none, will submit this tomorrow morning (pacific time).

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
495–496 ↗(On Diff #212708)

These lines could probably be combined, but LGTM.

pirama updated this revision to Diff 213713.Aug 6 2019, 1:58 PM

Avoid extra variable

nickdesaulniers accepted this revision.Aug 6 2019, 2:03 PM
This revision was automatically updated to reflect the committed changes.
vsk added a subscriber: vsk.Nov 18 2019, 6:05 PM

Hi @pirama, I've bisected an internal build break down to this commit. We have code that pushes a .ascii directive through inline asm, then expects to find the .ascii directive in the resulting assembly (for weird rewriting purposes) [1]. Trouble is, with the '#' missing, the asm writer (MCAsmStreamer?) prefers to emit a .byte directive instead of the usual .ascii, breaking the weird makefile. Any idea how the old behavior might be recovered? (of course we'll try to just change the makefile, it's just that that can be slow)

[1]

// generator
#define DECLARE(SYM, VAL) \
  __asm("DEFINITION__define__" SYM ":\t .ascii \"%0\"" : : "i"  ((u_long)(VAL)))

// makefile
assym.s: genassym.o
	$(_v)sed -e '/^[[:space:]]*DEFINITION__define__/!d;{N;s/\n//;}' -e 's/^[[:space:]]*DEFINITION__define__\([^:]*\):.*ascii.*\"[\$$]*\([-0-9\#]*\)\".*$$/#define \1 \2/' -e 'p'  -e 's/#//2' -e 's/^[[:space:]]*#define \([A-Za-z0-9_]*\)[[:space:]]*[\$$#]*\([-0-9]*\).*$$/#define \1_NUM \2/' genassym.o > $@

-fno-integrated-as should make the exact text of inline asm statements show up in the assembly file, if that's what you want.

So you could maybe do:

// generator
#define DECLARE(SYM, VAL) \
  __asm("DEFINITION__define__" SYM ":\t .ascii #\"%0\"" : : "i"  ((u_long)(VAL)))

// makefile
assym.s: genassym.o
	$(_v)sed -e '/^[[:space:]]*DEFINITION__define__/!d;{N;s/\n//;}' -e 's/^[[:space:]]*DEFINITION__define__\([^:]*\):.*ascii.*\"[#\$$]*\([-0-9\#]*\)\".*$$/#define \1 \2/' -e 'p'  -e 's/#//2' -e 's/^[[:space:]]*#define \([A-Za-z0-9_]*\)[[:space:]]*[\$$#]*\([-0-9]*\).*$$/#define \1_NUM \2/' genassym.o > $@

(That's an extra # in the asm and in the makefile).
It's easier to mentally parse that Makefile statement if you split on -e.

vsk added a comment.Nov 19 2019, 10:25 AM

Thanks Nick & Eli, '-fno-integrated-as' does the trick, as does changing the makefile. If the effect of this patch on the asm writer is intentional/correct, we can just push for a project change instead of fiddling with the compiler.

In D65550#1752082, @vsk wrote:

Thanks Nick & Eli, '-fno-integrated-as' does the trick, as does changing the makefile. If the effect of this patch on the asm writer is intentional/correct, we can just push for a project change instead of fiddling with the compiler.

@vsk The switch from .ascii to .byte was not intentional. But I'm not sure if it's incorrect. Can someone familiar with the AArch64 ASM confirm if this change doesn't inadvertently use the wrong directive?

The following two statements are equivalent:

.ascii "a"
.byte 97

It looks like the integrated assembler "translates" from the first to the second in inline asm, due to the way the MC layer APIs work. Probably not intentional, but I don't think it's a problem.