[x86/asm] Make variants work when converting at&t inline asm input to intel asm output

[thakis]

Summary

asm always has AT&T-style input (asm inteldialect has Intel-style asm
input), so EmitGCCInlineAsmStr() always has to pick the same variant since it
cares about the input asm string, not the output asm string.

For PowerPC, that default variant is 1. For other targets, it's 0.

Without this, the included test case errors out with

error: unknown use of instruction mnemonic without a size suffix
         mov rax, rbx

since it picks the intel branch and then tries to interpret it as AT&T
when selecting intel-style output with -x86-asm-syntax=intel.

[hans]

Looks reasonable to me, but every time I touch anything related to inline asm, something always breaks, so I'm also a bit worried.

The IBM folks touched this line not long ago (https://reviews.llvm.org/D98276), so let's bring them in on this.

[thakis]

https://reviews.llvm.org/D98276 just moved things around, so I think it's mostly independent of this change.

But it looks like llvm/test/CodeGen/PowerPC/asm-dialect.ll is failing with this if the ppc target is enabled.

Related changes there are https://reviews.llvm.org/rGe21237e59a8e172e4b038c4998411538853d6bc5 and https://reviews.llvm.org/rG266db7fe043f8f29bc76a7242761d35b3e70cd82.

It looks like the only backends that have tests for variants checked in are ppc and x86. It looks like ppc always wants variant 1, while x86 always wants variant 0 for asm and always variant 1 for asm inteldialect.

It does look like MAI->getAssemblerDialect() is generally more used as the dialect for writing. So in general, I think reading it here is incorrect, since variant selection is more a property of the contents of the asm string.

So I think the thing to do here is probably to do something like int AsmPrinterVariant = MMI->getTarget().UnqualifiedInlineAsmVariant(); and add LLVMTargetMachine::UnqualifiedInlineAsmVariant() that returns 0 but make it return 1 in PPCTargetMachine.

(https://reviews.llvm.org/rG1e9097e36a7acdcd75a2f8f229275afd33542db3 is also somewhat related.)

[thakis]

new approach to unbreak powerpc

[uweigand]

Thanks @hans for the heads-up, I do indeed think this patch would break inline asm on z/OS.

This seems to be the same underlying problem I had raised earlier this year in this patch (which never went anywhere): https://reviews.llvm.org/D95444 . To quote:

LLVM has two notions of "assembler dialect" that are conceptually distinct, but are currently somewhat intermixed.

The first notion corresponds to platform-specific syntactical differences in assembler text, e.g. the distinction between AT&T and Intel-style assembler for x86 (and similar differences on other architectures). The second notion corresponds to C/C++ source level differences in handling the inputs/outputs of *inline* assembler statements, specifically the difference between GNU inline asm and MS inline asm.

These are conceptually different, but the current inline assembler handling always enforces AT&T style assembler with GNU inline asm and Intel-style assembler with MS inline asm. This is done even on non-Intel platforms, which may not even have AT&T vs. Intel assembler -- that's not an issue for MS inline asm, as this itself only exists on Intel, but it is a problem if a platform wants to use different assembler styles for *GNU* inline asm. (Specifically, we want to do this on SystemZ to distinguish between GAS and HLASM style assembler text.)

Maybe some of the ideas in the above patch can be re-activated to fix this particular Intel problem without breaking z/OS?

[thakis]

In D113894#3131809, @uweigand wrote:

Thanks @hans for the heads-up, I do indeed think this patch would break inline asm on z/OS.

Do you have any test coverage of z/OS? All tests pass with this as far as I can tell.

[uweigand]

In D113894#3131822, @thakis wrote:

In D113894#3131809, @uweigand wrote:

Thanks @hans for the heads-up, I do indeed think this patch would break inline asm on z/OS.

Do you have any test coverage of z/OS? All tests pass with this as far as I can tell.

The z/OS test coverage is still a bit weak -- @Kai, do we have inline assembler tests in the tree already?

[thakis]

Alternatively, you can tell me which command to run manually to test.

(But in general, if there isn't test coverage, expect others to break you.)

[Kai]

The z/OS test coverage is still a bit weak -- @Kai, do we have inline assembler tests in the tree already?

No, there are currently no inline assembler tests in tree. We are still working on it.

[thakis]

Sounds like this can land as-is then and you'll figure it out, yes?

[craig.topper]

I think gcc's documentation says that inline assembly parsing variant follows -masm?

If the integrated assembler is disabled, I would think we need to follow -masm to match the dialect of the non inline assembly?

[thakis]

In D113894#3132343, @craig.topper wrote:

I think gcc's documentation says that inline assembly parsing variant follows -masm?

Right, that's what I'm trying to make happen (in D113707), and this is one of the prereqs to making the llvm bits work for that. With that patch, TUs compiled with -masm=intel will emit asm inteldialect instrs for inline asm.

[craig.topper]

In D113894#3132406, @thakis wrote:

In D113894#3132343, @craig.topper wrote:

I think gcc's documentation says that inline assembly parsing variant follows -masm?

Right, that's what I'm trying to make happen (in D113707), and this is one of the prereqs to making the llvm bits work for that. With that patch, TUs compiled with -masm=intel will emit asm inteldialect instrs for inline asm.

I thought asm inteldialect is used to select EmitMSInlineAsmStr in AsmPrinterInlineAsm.cpp. Is that also changing? Are the only differences between those functions related to the assembly syntax and not the originating C code?

[thakis]

I thought asm inteldialect is used to select EmitMSInlineAsmStr in AsmPrinterInlineAsm.cpp. Is that also changing? Are the only differences between those functions related to the assembly syntax and not the originating C code?

That's true, and that's not changing, at least that's my current plan. D113932 is the last change in LLVM land I want to make in preparation for the clang change.

The idea is that att style __asm__ becomes asm and -masm=intel __asm__ becomes asm inteldialect.

I'm not super duper familiar with the asm code (also not 100% unfamiliar though), so maybe there's some better way to handle this? But this approach seems to work well in my tests, seems to pass tests, and is fairly non-invasive.

[thakis]

It's important to keep _output_ and _input_ asm dialects mentally separated. At least that's confusing to me :)

asm vs asm inteldialect controls the dialect in inline asm inputs. Either of those can output at&t or intel asm at the end (controlled by the -x86-asm-syntax= llvm option).

[thakis]

In D113894#3132622, @thakis wrote:

I thought asm inteldialect is used to select EmitMSInlineAsmStr in AsmPrinterInlineAsm.cpp. Is that also changing? Are the only differences between those functions related to the assembly syntax and not the originating C code?

That's true, and that's not changing, at least that's my current plan. D113932 is the last change in LLVM land I want to make in preparation for the clang change.

I've now uploaded an updated version of D113707, the clang change. Maybe looking at that makes it clearer how the three changes are related :)

[hans]

lgtm

llvm/include/llvm/Target/TargetMachine.h

public:
   /// registers, except that variadic defs will be allocated vregs.
   virtual bool usesPhysRegsForValues() const { return true; }
 
   /// True if the target wants to use interprocedural register allocation by
   /// default. The -enable-ipra flag can be used to override this.
   virtual bool useIPRA() const {
     return false;
   }
+
+  /// The default variant to use in unqualified `asm` instructions.
+  /// If this returns 0, `asm "$(foo$|bar$)"` will evaluate to `asm "foo"`.
+  virtual int unqualifiedInlineAsmVariant() const { return 0; }
 };
 
 /// Helper method for getting the code model, returning Default if
 /// CM does not have a value. The tiny and kernel models will produce
 /// an error, so targets that support them or require more complex codemodel
 /// selection logic should implement and call their own getEffectiveCodeModel.
 inline CodeModel::Model getEffectiveCodeModel(Optional<CodeModel::Model> CM,
                                               CodeModel::Model Default) {

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

 
 static void EmitGCCInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
                                 MachineModuleInfo *MMI, const MCAsmInfo *MAI,
                                 AsmPrinter *AP, uint64_t LocCookie,
                                 raw_ostream &OS) {
   int CurVariant = -1;            // The number of the {.|.|.} region we are in.
   const char *LastEmitted = AsmStr; // One past the last character emitted.
   unsigned NumOperands = MI->getNumOperands();
-  int AsmPrinterVariant = MAI->getAssemblerDialect();
+  int AsmPrinterVariant = MMI->getTarget().unqualifiedInlineAsmVariant();
 
   if (MAI->getEmitGNUAsmStartIndentationMarker())
     OS << '\t';
 
   while (*LastEmitted) {
     switch (*LastEmitted) {
     default: {
       // Not a special case, emit the string section literally.

llvm/lib/Target/PowerPC/PPCTargetMachine.h

public:
   };
 
   bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override {
     // Addrspacecasts are always noops.
     return true;
   }
 
   bool isLittleEndian() const;
+
+  int unqualifiedInlineAsmVariant() const override { return 1; }
 };
 } // end namespace llvm
 
 #endif

llvm/test/CodeGen/X86/asm-dialect.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu %s -o - \
+; RUN:     | FileCheck --check-prefix=OUTPUT_ATT %s
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu %s -x86-asm-syntax=intel -o - \
+; RUN:     | FileCheck --check-prefix=OUTPUT_INTEL %s
+
+define void @f() {
+; OUTPUT_ATT-LABEL: f:
+; OUTPUT_ATT:       # %bb.0:
+; OUTPUT_ATT-NEXT:    #APP
+; OUTPUT_ATT-NEXT:    movq %rbx, %rax
+; OUTPUT_ATT-NEXT:    #NO_APP
+; OUTPUT_ATT-NEXT:    retq
+;
+; OUTPUT_INTEL-LABEL: f:
+; OUTPUT_INTEL:       # %bb.0:
+; OUTPUT_INTEL-NEXT:    #APP
+; OUTPUT_INTEL-NEXT:    mov rax, rbx
+; OUTPUT_INTEL-NEXT:    #NO_APP
+; OUTPUT_INTEL-NEXT:    ret
+  call void asm sideeffect "$(movq %rbx, %rax $|mov rax, rbx$)", "~{dirflag},~{fpsr},~{flags}"()
+
+  ret void
+}