Page MenuHomePhabricator

[ThinLTO] Always parse module level inline asm with At&t dialect
ClosedPublic

Authored by hans on Jun 30 2020, 4:08 AM.

Details

Summary

clang-cl passes -x86-asm-syntax=intel to the cc1 invocation so that assembly listings produced by the /FA flag are printed in intel dialect. That flag however should not affect the *parsing* of inline assembly in the program. (See llvm.org/r322652)

When compiling normally, AsmPrinter::emitInlineAsm is used for assembling and defaults to At&t dialect. However, when compiling for ThinLTO, the code which parses module level inline asm to find symbols for the symbol table was failing to set the dialect. This patch fixes that. (See PR46503)

Diff Detail

Event Timeline

hans created this revision.Jun 30 2020, 4:08 AM
thakis accepted this revision.Jun 30 2020, 7:09 AM
This revision is now accepted and ready to land.Jun 30 2020, 7:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 1:03 AM

Hi @hans , we're having some issues with using the AssemblerDialect mechanism to support both the GNU assembler and the IBM HLASM assembler in the SystemZ back-end. See also the discussion started here: https://lists.llvm.org/pipermail/llvm-dev/2020-November/146875.html

One of the issues that showed up is what you're refering to above:

That flag however should not affect the *parsing* of inline assembly in the program.

I'm wondering why this is true. I mean, it is true that the flag currently does not affect parsing of inline asm, but I'm wondering whether it *should* be that way.

Note that the -x86-asm-syntax=intel LLVM flag is used to implement the -masm=intel clang command line option, which exists also in GCC and where hopefully the two compilers should be compatible. But in GCC, that flag is documented to affect parsing of inline assembly just like it affects output. See https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/x86-Options.html#x86-Options

-masm=dialect

Output assembly instructions using selected dialect. Also affects which dialect is used for basic asm (see Basic Asm) and extended asm (see Extended Asm). Supported choices (in dialect order) are ‘att’ or ‘intel’. The default is ‘att’. Darwin does not support ‘intel’.

What is the reason for treating this differently in LLVM?

hans added a comment.Thu, Jan 14, 10:15 AM

Hi @hans , we're having some issues with using the AssemblerDialect mechanism to support both the GNU assembler and the IBM HLASM assembler in the SystemZ back-end. See also the discussion started here: https://lists.llvm.org/pipermail/llvm-dev/2020-November/146875.html

One of the issues that showed up is what you're refering to above:

That flag however should not affect the *parsing* of inline assembly in the program.

I'm wondering why this is true. I mean, it is true that the flag currently does not affect parsing of inline asm, but I'm wondering whether it *should* be that way.

Note that the -x86-asm-syntax=intel LLVM flag is used to implement the -masm=intel clang command line option, which exists also in GCC and where hopefully the two compilers should be compatible. But in GCC, that flag is documented to affect parsing of inline assembly just like it affects output. See https://gcc.gnu.org/onlinedocs/gcc-10.2.0/gcc/x86-Options.html#x86-Options

-masm=dialect

Output assembly instructions using selected dialect. Also affects which dialect is used for basic asm (see Basic Asm) and extended asm (see Extended Asm). Supported choices (in dialect order) are ‘att’ or ‘intel’. The default is ‘att’. Darwin does not support ‘intel’.

What is the reason for treating this differently in LLVM?

The motivation for my change was really just to make ThinLTO compiles work the same as non-ThinLTO ones.

Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently in that way, but that certainly suggests there's a problem here.

(From clang-cl's point of view we just want to set the *output* dialect to Intel, but we could invent a different flag for that if necessary.)

What is the reason for treating this differently in LLVM?

I'm not sure if it is related to this. I think one difference is that LLVM is supporting MS inline assembly. Although it uses Intel dialect, it has different representation in input, output, clobber etc. with GCC'.

What is the reason for treating this differently in LLVM?

I'm not sure if it is related to this. I think one difference is that LLVM is supporting MS inline assembly. Although it uses Intel dialect, it has different representation in input, output, clobber etc. with GCC'.

That is indeed another complication. On the one hand, we have two different inline asm formats, GNU assembly and MS assembly. These differ completely in how asm arguments (passed in from surrounding C/C++ code) are handled and therefore need to be parsed quite differently. On the other hand, we have the different assembler dialects (AT&T vs. Intel on x86), which affect how the single asm instructions are to be parsed.

Unfortunately, the distinction between the two seems to be somewhat muddled in LLVM at this point. In particular, MS assembly statements are represented at the LLVM IR level via the "inteldialect" token. This is a bit confusing because while MS asm indeed always uses the Intel dialect, we can also have GNU asm with the Intel dialect -- at least this is the case in GCC when using -masm=intel.

What I think we should have is:

  • the LLVM IR level distinction between GNU and MS asm statements; and in addition
  • for GNU asm statements, a target-specific selection of assembler variants, which may depend on the target triple and/or command line options like -masm=

If you look at AsmPrinter::emitInlineAsm, this is actually partially implemented already:

// The variant of the current asmprinter.
int AsmPrinterVariant = MAI->getAssemblerDialect();
AsmPrinter *AP = const_cast<AsmPrinter*>(this);
if (MI->getInlineAsmDialect() == InlineAsm::AD_ATT)
  EmitGCCInlineAsmStr(AsmStr, MI, MMI, AsmPrinterVariant, AP, LocCookie, OS);
else
  EmitMSInlineAsmStr(AsmStr, MI, MMI, AP, LocCookie, OS);

So here the LLVM IR marker is used to select between GCC and MS inline asm instructions, and for the GCC inline asm statements, the target is queried as to the proper asm dialect variant to use.

However, later on we have a

Parser->setAssemblerDialect(Dialect);

call, where the Dialect is again taken from the LLVM IR setting -- so for GNU asm, this gets now always set back to AT&T. It seems to me that this is simply wrong.

Then, we finally have module level inline asm statements, which are always treated as GNU style (although this may not really make any difference since module level statements do not have arguments anyway). Again because of the above setAssemblerDialect call they would therefore be also treated as AT&T dialect, which I guess is what @hans originally noticed.

The motivation for my change was really just to make ThinLTO compiles work the same as non-ThinLTO ones.

Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently in that way, but that certainly suggests there's a problem here.

So I'm wondering, if I remove the above setAssemblerDialect line and revert this commit, we should have inline asm (both module-level and GNU function-leve) respect the target-selected asm dialect variant both for ThinLTO and non-ThinLTO, so they should match again. Would that also solve the problem you were originally tracking?

hans added a subscriber: rnk.Fri, Jan 15, 4:33 AM

The motivation for my change was really just to make ThinLTO compiles work the same as non-ThinLTO ones.

Maybe the fact that -x86-asm-syntax=intel doesn't affect inline asm is a bug. I wasn't aware that Clang and GCC's -masm= flags behaved differently in that way, but that certainly suggests there's a problem here.

So I'm wondering, if I remove the above setAssemblerDialect line and revert this commit, we should have inline asm (both module-level and GNU function-leve) respect the target-selected asm dialect variant both for ThinLTO and non-ThinLTO, so they should match again. Would that also solve the problem you were originally tracking?

Not completely, because clang-cl sets -x86-asm-syntax=intel to enable intel-style asm in assembly listing output. We'd have to find another way of doing that without affecting the inline asm dialect.