Page MenuHomePhabricator

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

Authored by pirama on Wed, Jul 31, 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.Wed, Jul 31, 5:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jul 31, 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.Mon, Aug 5, 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.Tue, Aug 6, 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.Tue, Aug 6, 2:14 AM
pirama added a comment.Tue, Aug 6, 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.Tue, Aug 6, 1:58 PM

Avoid extra variable

nickdesaulniers accepted this revision.Tue, Aug 6, 2:03 PM
This revision was automatically updated to reflect the committed changes.