Demote EmitRawText call in AsmPrinter::EmitInlineAsm() and remove hasRawTextSupport() call

[dsanders]

Summary

AsmPrinter::EmitInlineAsm() will no longer use the EmitRawText() call for targets with mature MC support. Such targets will always parse the inline assembly (even when emitting assembly). Targets without mature MC support continue to use EmitRawText() for assembly output.

The hasRawTextSupport() check in AsmPrinter::EmitInlineAsm() has been replaced with MCAsmInfo::UseIntegratedAs which when true, causes the integrated assembler to parse inline assembly (even when emitting assembly output). UseIntegratedAs is set to true for targets that consider any failure to parse valid assembly to be a bug. Target specific subclasses generally enable the integrated assembler in their constructor. The default value can be overridden with -no-integrated-as.

All tests that rely on inline assembly supporting invalid assembly (for example, those that use mnemonics such as 'foo' or 'hello world') have been updated to disable the integrated assembler.

[dsanders]

Use llvm::Triple::getArch() instead of matching triples directly.

[dsanders]

Minor tweaks

[dsanders]

Here's my patch, but there's a problem with it.

As-is, it causes ~50 test failures. The ones I've looked at so far are generally due to directives such as .foo_directive or .bork_directive in inline assembly. I could go through and change each test to use valid assembly, but I think a better solution would be to have isIntegratedAsDefault() and useIntegratedAs() as the predicates and then have an equivalent to -no-integrated-as force useIntegratedAs() to return false. This is the same model as clang uses.

The effective change to the plan we previously discussed is that there is a way to force the use of EmitRawText() using -no-integrated-as. The default behaviour where targets with mature MC's always parse inline assembly remains the same.

[dsanders]

Switched to isIntegratedAsDefault() and useIntegratedAs() method.
Added -no-integrated-inline-as to llc and used it where tests required invalid
assembly, magic comments, or non-canonical output.

[• rafael]

This is certainly going on the right direction. Thank you so much for working on this!

include/llvm/MC/MCSubtargetInfo.h

48

Why MCSubtargeInfo? I have the impression that MCAsmInfo might be a better match. Its initialization structure is a bit more verbose, but being split among the targets matches what clang does a bit better and should allow each target to change the rules for when they use the integrated assembler without touching common code.

73

Don't duplicate the name on the comment.

lib/MC/MCSubtargetInfo.cpp

21

If going with MCasmInfo, I would suggest adding a set method to it. That way the constructor set the default and LLVMTargetMachine::initAsmInfo can call the setter if -no-integrated-inline-as is passed.

129

The clang logic has more cases. For example, I think the ppc assembler is used on some BSDs.
This is probably fine for now. Just change the FIXME to say that this version should be synced with clang and clang changed to use this so that we have a single implementation.

[dsanders]

Thanks for the review.

In addition to the changes suggested by comments, I'm going to drop the word 'Inline' from the variables and options. Otherwise it will be misleading when clang/llvm's logic is unified.

include/llvm/MC/MCSubtargetInfo.h

48

I was thinking that the maturity of MC support potentially varies on a subtarget basis. For example, if MC support for MIPS32r2 is mature, it doesn't follow that MC support for MIPS-I is also mature.

Having looked at MCAsmInfo, I agree that it's a better match and should still be able to handle any likely subtarget issues. It also has the nice benefit of keeping the targets separate in isIntegratedInlineAsDefault() since it's normally subclassed.

73

I did this because every other function in the file was doing the same thing. I'll remove it.

lib/MC/MCSubtargetInfo.cpp

129

Yes you're right. I think I've found all of them now.

[dsanders]

• Moved the predicates to MCAsmInfo.
• Implemented all (except one) of clangs conditions for whether -integrated-as is the default.
  • A couple conditions have imperfect conversions. For example testing for MachO output has become testing for Darwin.
  • getOS() == Solaris, is currently missing. There doesn't seem to be a good place to put it.
• Dropped the word 'Inline' from variables and option names. If this code is going to be unified with clang's logic then it will cover inline and non-inline asm.

[• rafael]

99% there.

Sorry for the delay with code review. The only real issue is the question about having two member variables instead of one.

include/llvm/MC/MCAsmInfo.h

309 ↗

(On Diff #6917)

Why do we need two variables? It seems that the constructors should be able to set UseIntegratedAssembler directly and LLVMTargetMachine.cpp can then override it if requested.

lib/MC/MCAsmInfo.cpp

96 ↗

(On Diff #6917)

IMHO it is the MCAsmInfoDarwin that should be named MCAsmInfoMachO. The handling of COFF and MachO are probably correct enough to not need a FIXME :-)

test/CodeGen/Generic/mature-mc-support.ll

2

nit: add something like "is parsed even when printing assembly".

[dsanders]

include/llvm/MC/MCAsmInfo.h

309 ↗

(On Diff #6917)

I was mostly following clang's lead for this. Clang doesn't seem to use the separation for much (the only bit I've found is that the default value is re-used as the default for -masm-verbose) so I'll change it to a single variable.

lib/MC/MCAsmInfo.cpp

96 ↗

(On Diff #6917)

Great. Do you have any thoughts on the 'OS is Solaris' check?

I think I can put it in SparcELFMCAsmInfo and X86ELFMCAsmInfo to cover most of it, but I'm not sure if there are other subclasses that need it.

[• rafael]

IMHO it is the MCAsmInfoDarwin that should be named MCAsmInfoMachO. The handling of COFF and MachO are probably correct enough to not need a FIXME :-)

Great. Do you have any thoughts on the 'OS is Solaris' check?

I think I can put it in SparcELFMCAsmInfo and X86ELFMCAsmInfo to cover most of it, but I'm not sure if there are other subclasses that need it.

That should do it. If people want in the future we can move it up to
MCAsmInfoELF by passing a triple down to the constructor.

Cheers,
Rafael

[dsanders]

• Removed IsIntegratedAssemblerDefault.
• Added missing 'OS is Solaris' check.
• Fixed nit.

[• rafael]

LGTM!

Thanks!

[dsanders]

Closed by commit rL201237 (authored by @dsanders).

include/llvm/MC/MCSubtargetInfo.h

class MCSubtargetInfo {
 
   const InstrStage *Stages;            // Instruction itinerary stages
   const unsigned *OperandCycles;       // Itinerary operand cycles
   const unsigned *ForwardingPaths;     // Forwarding paths
   unsigned NumFeatures;                // Number of processor features
   unsigned NumProcs;                   // Number of processors
   uint64_t FeatureBits;                // Feature bits for current CPU + FS
 
+  bool HasMatureMCSupport; // MC Support is mature for this subtarget
+
 public:
   void InitMCSubtargetInfo(StringRef TT, StringRef CPU, StringRef FS,
                            const SubtargetFeatureKV *PF,
                            const SubtargetFeatureKV *PD,
                            const SubtargetInfoKV *ProcSched,
                            const MCWriteProcResEntry *WPR,
                            const MCWriteLatencyEntry *WL,
                            const MCReadAdvanceEntry *RA,
                            const InstrStage *IS,
                            const unsigned *OC, const unsigned *FP,
                            unsigned NF, unsigned NP);
 
   /// getTargetTriple - Return the target triple string.
   StringRef getTargetTriple() const {
     return TargetTriple;
   }
 
   /// getFeatureBits - Return the feature bits.
   ///
   uint64_t getFeatureBits() const {
     return FeatureBits;
   }
 
+  /// hasMatureMCSupport - Return true if the MC support is considered to be
+  /// mature. MC support is mature when 'llc -filetype=obj' failing to handle
+  /// a valid piece of assembly (inline or otherwise) is considered a bug.
+  bool hasMatureMCSupport() const { return HasMatureMCSupport; }
+
+  // Determine MC layer maturity by target triple
+  bool getMCMaturity(StringRef TT) const;
+
   /// InitMCProcessorInfo - Set or change the CPU (optionally supplemented with
   /// feature string). Recompute feature bits and scheduling model.
   void InitMCProcessorInfo(StringRef CPU, StringRef FS);
 
   /// InitCPUSchedModel - Recompute scheduling model based on CPU.
   void InitCPUSchedModel(StringRef CPU);
 
   /// ToggleFeature - Toggle a feature and returns the re-computed feature

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

void AsmPrinter::EmitInlineAsm(StringRef Str, const MDNode *LocMDNode,
                                InlineAsm::AsmDialect Dialect) const {
   assert(!Str.empty() && "Can't emit empty inline asm block");
 
   // Remember if the buffer is nul terminated or not so we can avoid a copy.
   bool isNullTerminated = Str.back() == 0;
   if (isNullTerminated)
     Str = Str.substr(0, Str.size()-1);
 
-  // If the output streamer is actually a .s file, just emit the blob textually.
+  // If the output streamer does not have mature MC support, just emit the blob
+  // textually. Otherwise parse the asm and emit it via MC support.
   // This is useful in case the asm parser doesn't handle something but the
   // system assembler does.
-  if (OutStreamer.hasRawTextSupport()) {
+  if (!TM.getSubtarget<MCSubtargetInfo>().hasMatureMCSupport()) {
     OutStreamer.EmitRawText(Str);
     emitInlineAsmEnd(TM.getSubtarget<MCSubtargetInfo>(), 0);
     return;
   }
 
   SourceMgr SrcMgr;
   SrcMgrDiagInfo DiagInfo;
 

lib/MC/MCSubtargetInfo.cpp

 #include "llvm/MC/MCInstrItineraries.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace llvm;
 
 MCSchedModel MCSchedModel::DefaultSchedModel; // For unknown processors.
 
 /// InitMCProcessorInfo - Set or change the CPU (optionally supplemented
 /// with feature string). Recompute feature bits and scheduling model.
 void
 MCSubtargetInfo::InitMCProcessorInfo(StringRef CPU, StringRef FS) {
   SubtargetFeatures Features(FS);
   FeatureBits = Features.getFeatureBits(CPU, ProcDesc, NumProcs,
                                         ProcFeatures, NumFeatures);
 
MCSubtargetInfo::InitMCSubtargetInfo(StringRef TT, StringRef CPU, StringRef FS,
   ReadAdvanceTable = RA;
 
   Stages = IS;
   OperandCycles = OC;
   ForwardingPaths = FP;
   NumFeatures = NF;
   NumProcs = NP;
 
+  HasMatureMCSupport = getMCMaturity(TT);
+
   InitMCProcessorInfo(CPU, FS);
 }
 
 /// ToggleFeature - Toggle a feature and returns the re-computed feature
 /// bits. This version does not change the implied bits.
 uint64_t MCSubtargetInfo::ToggleFeature(uint64_t FB) {
   FeatureBits ^= FB;
   return FeatureBits;
MCSubtargetInfo::getInstrItineraryForCPU(StringRef CPU) const {
   return InstrItineraryData(SchedModel, Stages, OperandCycles, ForwardingPaths);
 }
 
 /// Initialize an InstrItineraryData instance.
 void MCSubtargetInfo::initInstrItins(InstrItineraryData &InstrItins) const {
   InstrItins =
     InstrItineraryData(CPUSchedModel, Stages, OperandCycles, ForwardingPaths);
 }
+
+bool MCSubtargetInfo::getMCMaturity(StringRef TT) const {
+  Triple Trip(TT);
+
+  if (Trip.getArch() == llvm::Triple::aarch64 ||
+      Trip.getArch() == llvm::Triple::arm ||
+      Trip.getArch() == llvm::Triple::x86 ||
+      Trip.getArch() == llvm::Triple::x86_64 ||
+      Trip.getArch() == llvm::Triple::thumb)
+    return true;
+
+  // Assume the MC layer is mature.
+  return false;
+}
+

test/CodeGen/Generic/mature-mc-support.ll

+; Test that inline assembly is parsed by the MC layer when MC support is mature.
+
+; RUN: not llc -march=aarch64 < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=aarch64 -filetype=obj < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=arm < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=arm -filetype=obj < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=thumb < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=thumb -filetype=obj < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=x86 < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=x86 -filetype=obj < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=x86-64 < %s 2>&1 | FileCheck %s
+; RUN: not llc -march=x86-64 -filetype=obj < %s 2>&1 | FileCheck %s
+
+module asm " .this_directive_is_very_unlikely_to_exist"
+
+; CHECK: LLVM ERROR: Error parsing inline asm