[PATCH] D21809: [SystemZ] Add support for .insn and .word/.short/.long/.quad directives

[Marcin Kościelnicki via llvm-commits]

koriakin added inline comments.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:545
@@ +544,3 @@
+static const InstrInfoEntry InstrInfoTable[] = {
+  /* { InstrFormat, NumClasses, Size, Shift, Classes } */
+  { InsnE,    0, 2, {}, {} },
----------------
should be NumOperands

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:549
@@ +548,3 @@
+    { RegisterOperand, ImmOperand } },
+  { InsnRIE,  3, 6, { 36, 32, 16 },
+    { RegisterOperand, RegisterOperand, ImmOperand } },
----------------
For better or worse, gas considers the immediate here to be pc-relative and shifted.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:551
@@ +550,3 @@
+    { RegisterOperand, RegisterOperand, ImmOperand } },
+  { InsnRIL,  2, 6, { 36, 0 },
+    { RegisterOperand, ImmOperand } },
----------------
Also pc-relative and shifted.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:569
@@ +568,3 @@
+    { RegisterOperand, RegisterOperand, AddrOperand } },
+  { InsnRSI,  3, 6, { 36, 32, 0 },
+    { RegisterOperand, RegisterOperand, ImmOperand } },
----------------
Likewise pc-relative and shifted.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:590
@@ +589,3 @@
+  { InsnSS,   3, 6, { 0, 0, 32 },
+    { AddrOperand, RegisterOperand } },
+  { InsnSSE,  2, 6, { 0, 0 },
----------------
This is wrong.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:592
@@ +591,3 @@
+  { InsnSSE,  2, 6, { 0, 0 },
+    { AddrOperand } },
+  { InsnSSF,  3, 6, { 0, 0, 36 },
----------------
Likewise.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:594
@@ +593,3 @@
+  { InsnSSF,  3, 6, { 0, 0, 36 },
+    { AddrOperand, RegisterOperand } }
+};
----------------
Likewise.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:697
@@ +696,3 @@
+    // The other formats don't have address operands.
+    assert(false && "Invalid address operand");
+    return true;
----------------
Looks like a job for llvm_unreachable.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:962
@@ +961,3 @@
+  for (uint32_t i = 0; i < Entry.NumOperands; i++) {
+    if (getLexer().isNot(AsmToken::Comma))
+      return TokError("unexpected token in directive");
----------------
Would be nice to have "too few operands" error here.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:972
@@ +971,3 @@
+      SMLoc StartLoc, EndLoc;
+      if (ParseRegister(Reg, StartLoc, EndLoc))
+        return Error(L, "error parsing register");
----------------
gas also accepts plain numbers whenever registers are accepted - while normally it's not much of an issue, it needs to be supported here, as the 'R' fields in many formats are also used for immediate arguments (eg. CC mask) for some instructions.

================
Comment at: lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp:996
@@ +995,3 @@
+      // Encode the immediate value.
+      Encoding |= (Imm << Shift);
+    }
----------------
Needs verification of immediate range (or at least clipping off the extra bits).


http://reviews.llvm.org/D21809