[PATCH] ARM IAS: support .inst directive

[Tim Northover]

  Hi Saleem,

  This looks pretty good, despite the comments I've added. One thing I'd say that didn't attach to any particular line is that a test for llvm-mc's text printing would be a good idea (i.e. ARMTargetAsmStreamer::emitInst).

  Cheers.

  Tim.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8330-8337
@@ +8329,10 @@
+      switch (Width) {
+      case 2:
+        if (Value->getValue() > 0xffff)
+          return Error(Loc, "inst.n operand is too big, use inst.w instead");
+        break;
+      case 4:
+        if (Value->getValue() > 0xffffffff)
+          Warning(Loc, "value 0x" + utohexstr(Value->getValue()) + " truncated");
+        break;
+      default:
----------------
It seems a little odd that "size == 2" always produces an error but "size == 4" a warning.

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:8303
@@ +8302,3 @@
+///  ::= .inst.w opcode [, ...]
+bool ARMAsmParser::parseDirectiveInst(SMLoc Loc, unsigned Width, char Suffix) {
+  if (isThumb()) {
----------------
I agree with Joerg that we probably shouldn't have redundant arguments like Width and Suffix. 

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:196
@@ -192,1 +195,3 @@
 
+void ARMTargetAsmStreamer::emitInst(uint32_t Inst, unsigned Size, char Suffix) {
+  OS << "\t.inst";
----------------
Size is unused in this function.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:393-394
@@ +392,4 @@
+       for (unsigned II = 0, IE = Size; II != IE; II = II + 2) {
+         const unsigned I0 = LittleEndian ? II + 0 : (Size - II - 1);
+         const unsigned I1 = LittleEndian ? II + 1 : (Size - II - 2);
+         Buffer[Size - II - 2] = uint8_t(Inst >> I0 * CHAR_BIT);
----------------
I believe instructions are always stored in little-endian format on ARM (well, approximately, it seems to be configurable in R-class processors to avoid annoying people with brain-dead legacy code: see A3.3.1).

If so, this logic is probably unnecessary.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:399
@@ +398,3 @@
+     } else {
+       assert(Size == 4 && "Size must be even");
+       EmitARMMappingSymbol();
----------------
I'm reasonably sure there are more even numbers than 4.

================
Comment at: lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:390
@@ +389,3 @@
+     if (IsThumb) {
+       assert(Size % 2 == 0 && "Size must be even");
+       EmitThumbMappingSymbol();
----------------
I think I'd prefer "Size == 2 || Size == 4".


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