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

Rafael Ávila de Espíndola rafael.espindola at gmail.com
Sun Feb 9 20:02:31 PST 2014


  99% there.

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


================
Comment at: include/llvm/MC/MCAsmInfo.h:309
@@ +308,3 @@
+    /// Should we use the integrated assembler?
+    bool UseIntegratedAssembler;
+
----------------
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.


================
Comment at: lib/MC/MCAsmInfo.cpp:96
@@ +95,3 @@
+  // - Windows always enables the integrated assembler by default
+  //   - MCAsmInfoCOFF is handling this case, should it be MCAsmInfoMicrosoft?
+  // - MachO targets always enables the integrated assembler by default
----------------
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 :-)

================
Comment at: test/CodeGen/Generic/mature-mc-support.ll:1
@@ +1,2 @@
+; Test that inline assembly is parsed by the MC layer when MC support is mature.
+
----------------
nit: add something like "is parsed even when printing assembly".


http://llvm-reviews.chandlerc.com/D2686



More information about the llvm-commits mailing list