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

Daniel Sanders daniel.sanders at imgtec.com
Wed Feb 5 07:39:15 PST 2014


  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.


================
Comment at: include/llvm/MC/MCSubtargetInfo.h:74
@@ -70,1 +73,3 @@
 
+  /// useIntegratedInlineAs - Return true if the inline assembly should be
+  /// parsed.
----------------
Rafael Ávila de Espíndola wrote:
> Don't duplicate the name on the comment.
I did this because every other function in the file was doing the same thing. I'll remove it.

================
Comment at: include/llvm/MC/MCSubtargetInfo.h:48
@@ -47,1 +47,3 @@
 
+  bool UseIntegratedInlineAs; // Should we use the integrated assembler for
+                              // inline assembly on this subtarget.
----------------
Rafael Ávila de Espíndola wrote:
> 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.
> 
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.

================
Comment at: lib/MC/MCSubtargetInfo.cpp:136
@@ +135,3 @@
+  // This should be kept in sync with clang
+  if (Trip.getArch() == llvm::Triple::aarch64 ||
+      Trip.getArch() == llvm::Triple::arm ||
----------------
Rafael Ávila de Espíndola wrote:
> 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.
Yes you're right. I think I've found all of them now.


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



More information about the llvm-commits mailing list