This is an archive of the discontinued LLVM Phabricator instance.

[clang] Make -masm=intel affect inline asm style
ClosedPublic

Authored by thakis on Nov 11 2021, 12:57 PM.

Details

Summary

With this,

void f() {  __asm__("mov eax, ebx"); }

now compiles with clang with -masm=intel.

This matches gcc.

The flag is not accepted in clang-cl mode. It has no effect on
MSVC-style __asm {} blocks, which are unconditionally in intel
mode both before and after this change.

One difference to gcc is that in clang, inline asm strings are
"local" while they're "global" in gcc. Building the following with
-masm=intel works with clang, but not with gcc where the ".att_syntax"
from the 2nd asm() is in effect until file end (or until a
".intel_syntax" somewhere later in the file):

__asm__("mov eax, ebx");
__asm__(".att_syntax\nmovl %ebx, %eax");
__asm__("mov eax, ebx");

This also updates clang's intrinsic headers to work both in
-masm=att (the default) and -masm=intel modes.
The official solution for this according to "Multiple assembler dialects in asm
templates" in gcc docs->Extensions->Inline Assembly->Extended Asm
is to write every inline asm snippet twice:

bt{l %[Offset],%[Base] | %[Base],%[Offset]}

This works in LLVM after D113932 and D113894, so use that.
(Just putting .att_syntax at the start of the snippet works in some but not
all cases: When LLVM interpolates in parameters like %0, it uses at&t or
intel syntax according to the inline asm snippet's flavor, so the .att_syntax
within the snippet happens to late: The interpolated-in parameter is already
in intel style, and then won't parse in the switched .att_syntax)
It might be nice to invent a #pragma clang asm_dialect push "att" /
#pragma clang asm_dialect pop to be able to force asm style per snippet,
so that the inline asm string doesn't contain the same code in two variants,
but let's leave that for a follow-up.

Fixes PR21401 and PR20241.

Diff Detail

Event Timeline

thakis created this revision.Nov 11 2021, 12:57 PM
thakis requested review of this revision.Nov 11 2021, 12:57 PM
rnk added a comment.Nov 11 2021, 1:19 PM

This seems reasonable, but I worry about headers that contain AT&T style inline asm blobs, such as clang's own intrinsic headers. As you say, the directive is local. I would hate to end up in a place where we have to prefix every asm blob in clang/lib/Headers/*mmintrin.h with ".att_syntax on;".

Actually, git grep __asm__ ../clang/lib/Headers shows that there are not very many blobs. Maybe we should just prefix them.

Thanks for pointing out built-in headers, that's a great point. I started adding .att_syntax lines where needed, but the I realized that that doens't work: it's too late for things taking memory, since %0 replacement is done with asm writer setting before getting to reading the snippet. So %0 is replaced with (say) [eax] before things get to the asm parser, and then .att_syntax switches it to att syntax, and then it says "can't do [eax] in att syntax".

The official solution for this according to "Multiple assembler dialects in asm templates" in gcc docs->Extensions->Inline Assembly->Extended Asm is to write every inline asm snippet twice: bt{l %[Offset],%[Base] | %[Base],%[Offset]}. clang doesn't yet support {|}. So I think I have to update this patch to make clang support {|} and use that in the intrinsic headers where needed.

rnk added a comment.Nov 12 2021, 12:56 PM

One alternative would be to control this setting with a pragma stack, like we do for warnings, struct packing, etc. Something like:

// foo.h
#pragma clang asm_dialect push "att"
static inline ... foo { asm("..."); }
#pragma clang asm_dialect pop

The pragma really would just control the dialect on the inline asm blob, not the output assembly.

It seems more user friendly since they don't have to write two variants of their inline asm, but it is new and not compatible with any existing code.

thakis updated this revision to Diff 387381.Nov 15 2021, 1:18 PM
  • rebase on top of recent (partially still pending) llvm patches
  • use {...|...} in intrinsic headers
thakis edited the summary of this revision. (Show Details)Nov 15 2021, 1:27 PM

Fairly big update. I'd like to punt on the pragma for now since this became a lot more involved than expected already (see dependent patches; several of them have already landed).

thakis updated this revision to Diff 387399.Nov 15 2021, 1:57 PM

minor test expectations update

thakis edited the summary of this revision. (Show Details)Nov 15 2021, 1:59 PM
rnk added a comment.Nov 15 2021, 2:28 PM

Fairly big update. I'd like to punt on the pragma for now since this became a lot more involved than expected already (see dependent patches; several of them have already landed).

No problem.

This seems very reasonable.

thakis edited the summary of this revision. (Show Details)Nov 15 2021, 3:05 PM

(Here are a few revisions I read through while researching this patch:

da081a8e1f295ffdd670f87dba19f470a9a5acaa
and fc416faaa07c5
(and 4f2791617e2fa)
and aa23fa9f435d2

1778831a3d1d2 split the "ms" inline asm out

isSimple added in 19fe116fc0b35, meaningfully used in b737b6247f051)

hans accepted this revision.Nov 17 2021, 7:08 AM

lgtm

clang/lib/Headers/x86gprintrin.h
29–32

the 'l' suffix is probably not even needed for any of these movs?

clang/test/CodeGen/inline-asm-intel.c
4

Could the interesting flags (-triple x86_64-unknown-unknown -mllvm -x86-asm-syntax=att -inline-asm=intel) be put right at the beginning or end of the run lines? They're pretty long, so easy to get lost in the woods otherwise.

(I think the triple slash prefix is uncommon in clang tests.)

12

Maybe give these a comment too?

This revision is now accepted and ready to land.Nov 17 2021, 7:08 AM
This revision was landed with ongoing or failed builds.Nov 17 2021, 10:42 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 10:42 AM
thakis marked 3 inline comments as done.Nov 17 2021, 10:45 AM

Derp, I landed this without replying to the comments. I'll do a follow-up commit for them right now.

This contained a bug in that I didn't change CompilerInvocation::GenerateCodeGenArgs() to serialize the new flag correctly, so in build configs that do roundtripping the test failed.

I fixed the marshalling in https://reviews.llvm.org/rG36873fb768dbedeb3e9167117d3bb63b3720d214 (and https://reviews.llvm.org/rG3623163ae83799933bbeabe2cb3060537250c4b3), and I made https://reviews.llvm.org/D114120 to make sure more build configs do roundtripping (with that, I would've seen this locally instead of only on the bots).