[PATCH] Fix PR18345: ldr= pseudo instruction produces incorrect code when using in inline assembly

[David Peixotto]

  Rafael,

  The main reason that the AsmStreamer does not just print the ldr= pseudo and .ltorg directives is that the ldr= pseudo is an instruction rather than a directive. When we first designed the feature, it was important that the ldr instruction be parsed as a real instruction. There is no corresponding MCOperand for the ldr= pseudo once it has been parsed. That is we want to parse ldr r0, =0x1 as 'ldr r0, .LabelThatHolds0x1' so we do not create an instruction with a "fake" operand. In that regard the ldr= pseudo is similar to how we handle macros, they are expanded rather than just printed out.

  Based on your feedback, my plan is to get rid of the MachOStreamer and just add the llvm_unreachable implementations to the ARMTargetStreamer.


================
Comment at: include/llvm/MC/MCStreamer.h:85
@@ -80,3 +84,3 @@
 // FIXME: declared here because it is used from
 // lib/CodeGen/AsmPrinter/ARMException.cpp.
 class ARMTargetStreamer : public MCTargetStreamer {
----------------
Rafael Ávila de Espíndola wrote:
> I wonder if we should keep only the methods used by ARMException.cpp in here. Or maybe the debug info emission needs its own virtual interface.
> 
> Just thinking to myself, it we do that it should be in an another patch :-)
> 
Yeah, I saw the fixme, but wasn't quite sure how to fix it :) It would be nice to not expose the details of the ARMTargetStreamer here.

================
Comment at: include/llvm/MC/MCStreamer.h:116
@@ +115,3 @@
+  // Callbacks to handle assembler-generated constant pools
+  /// finish() - write out any non-empty assembler constant pools.
+  virtual void finish();
----------------
Rafael Ávila de Espíndola wrote:
> That is what the implementation does, not what the interface is for. I would move this comment to the implementation.
Ok, I will clean it up in the next patch.

================
Comment at: include/llvm/MC/MCStreamer.h:119
@@ +118,3 @@
+
+  /// addConstantPoolEntry - add a new entry to the constant pool
+  ///   for the current section and return an MCExpr that can be
----------------
Rafael Ávila de Espíndola wrote:
> Don't repeat the function name. Calling this .emitLDR would better match the convention of using the asm directive name. At least mention the directive in the comment.
> 
Ok, I will fixup the comment.

Part of the problem I had with naming is that these are not all directives. The ldr= pseudo is an instruction (e.g. ldr r0, =0x1). I chose the names to reflect the action taken on the internal constant pool.

================
Comment at: include/llvm/MC/MCStreamer.h:126
@@ +125,3 @@
+  /// the current section
+  void emitCurrentConstantPool();
+
----------------
Rafael Ávila de Espíndola wrote:
> emitLtorg or emitPool would better match the convention of naming after the assembly directive. At least mention the asm directives in the comment if you prefer  emitCurrentConstantPool.
Will do.

================
Comment at: test/CodeGen/ARM/inlineasm-ldr-pseudo.ll:2
@@ +1,3 @@
+; PR18354
+; RUN: llc -mtriple=arm-none-linux   < %s -filetype=obj | llvm-objdump -d - | FileCheck %s
+; RUN: llc -mtriple=arm-apple-darwin < %s -filetype=obj | llvm-objdump -d - | FileCheck %s
----------------
Rafael Ávila de Espíndola wrote:
> As much as I hate to admit it, this is a case where using "llc -filetype=obj" is the correct test :-(
> 
> Our handling of inline assembly just forwards it to the output, so for now we need this.
Yes, I agree. I first tried outputting the assembly, but as you said it does not show the problem because we just directly print the assembly as a text string.


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