Differential D65550
[AArch64] Do not emit '#' before immediates in inline asm Authored by pirama on Jul 31 2019, 5:44 PM.
Details The A64 assembly language does not require the '#' character to __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
Diff Detail
Event TimelineComment Actions 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 '#':
Some references: 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>]. Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions 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. Comment Actions Thanks Peter. I'll wait for another day for further comments, and if none, will submit this tomorrow morning (pacific time).
Comment Actions 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 > $@Comment Actions -fno-integrated-as should make the exact text of inline asm statements show up in the assembly file, if that's what you want. Comment Actions 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). Comment Actions 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. Comment Actions @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? Comment Actions 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. | ||||||||||
These lines could probably be combined, but LGTM.