[PATCH] D53816: [TableGen:AsmWriter] Cope with consecutive tied operands.

[Simon Tatham via Phabricator via llvm-commits]

simon_tatham created this revision.
simon_tatham added reviewers: fhahn, rengolin.
Herald added subscribers: llvm-commits, kristof.beyls, javed.absar.

When you define an instruction alias as a subclass of InstAlias, you
specify all the MC operands for the instruction it expands to, except
for operands that are tied to a previous one, which you leave out in
the expectation that the Tablegen output code will fill them in
automatically.

But the code in Tablegen's AsmWriter backend that skips over a tied
operand was doing it using 'if' instead of 'while', because it wasn't
expecting to find two tied operands in sequence.

So if an instruction updates a pair of registers in place, so that its
MC representation has two input operands tied to the output ones (for
example, Arm's UMLAL instruction), then any alias which wants to
expand to a special case of that instruction is likely to fail to
match, because the indices of subsequent operands will be off by one
in the generated printAliasInstr function.

This patch re-indents some existing code, so it's clearest when
viewed as a diff with whitespace changes ignored.


Repository:
  rL LLVM

https://reviews.llvm.org/D53816

Files:
  test/TableGen/ConsecutiveTiedAlias.td
  utils/TableGen/AsmWriterEmitter.cpp


Index: utils/TableGen/AsmWriterEmitter.cpp
===================================================================
--- utils/TableGen/AsmWriterEmitter.cpp
+++ utils/TableGen/AsmWriterEmitter.cpp
@@ -835,15 +835,20 @@
       for (unsigned i = 0, e = LastOpNo; i != e; ++i) {
         // Skip over tied operands as they're not part of an alias declaration.
         auto &Operands = CGA.ResultInst->Operands;
-        unsigned OpNum = Operands.getSubOperandNumber(MIOpNum).first;
-        if (Operands[OpNum].MINumOperands == 1 &&
-            Operands[OpNum].getTiedRegister() != -1) {
-          // Tied operands of different RegisterClass should be explicit within
-          // an instruction's syntax and so cannot be skipped.
-          int TiedOpNum = Operands[OpNum].getTiedRegister();
-          if (Operands[OpNum].Rec->getName() ==
-              Operands[TiedOpNum].Rec->getName())
-            ++MIOpNum;
+        while (true) {
+          unsigned OpNum = Operands.getSubOperandNumber(MIOpNum).first;
+          if (Operands[OpNum].MINumOperands == 1 &&
+              Operands[OpNum].getTiedRegister() != -1) {
+            // Tied operands of different RegisterClass should be explicit within
+            // an instruction's syntax and so cannot be skipped.
+            int TiedOpNum = Operands[OpNum].getTiedRegister();
+            if (Operands[OpNum].Rec->getName() ==
+                Operands[TiedOpNum].Rec->getName()) {
+              ++MIOpNum;
+              continue;
+            }
+          }
+          break;
         }
 
         std::string Op = "MI->getOperand(" + utostr(MIOpNum) + ")";
Index: test/TableGen/ConsecutiveTiedAlias.td
===================================================================
--- /dev/null
+++ test/TableGen/ConsecutiveTiedAlias.td
@@ -0,0 +1,49 @@
+// RUN: llvm-tblgen -gen-asm-writer -I %p/../../include %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def TestTarget : Target;
+
+class Encoding : Instruction {
+  field bits<4> Inst;
+}
+
+class TestReg<string name, bits<1> enc> : Register<name, []> {
+    let HWEncoding{15-1} = 0;
+    let HWEncoding{0} = enc;
+}
+
+def R0 : TestReg<"R0", 0>;
+def R1 : TestReg<"R1", 1>;
+def Reg : RegisterClass<"TestTarget", [i32], 32, (sequence "R%d", 0, 1)>;
+
+def DoubleUpdate : Encoding {
+  dag OutOperandList = (outs Reg:$dest1, Reg:$dest2);
+  dag InOperandList = (ins Reg:$dest1in, Reg:$dest2in, Reg:$src);
+  string AsmString = "update $dest1, $dest2, $src";
+  string AsmVariantName = "";
+  let Constraints = "$dest1 = $dest1in,$dest2 = $dest2in";
+  field bits<1> dest1;
+  field bits<1> dest2;
+  field bits<1> src;
+  let Inst{3} = 0;
+  let Inst{2} = dest1{0};
+  let Inst{1} = dest2{0};
+  let Inst{0} = src{0};
+}
+
+def DoubleUpdateAlias : InstAlias<
+   "alias $d, $s", (DoubleUpdate Reg:$d, Reg:$d, Reg:$s)>;
+
+// The DoubleUpdate instruction has five MC operands. Operands 0,1 are
+// the outputs $dest1 and $dest2, and operands 2,3,4 are the inputs
+// $dest1in, $dest2in and $src. Operand 2 is tied to 0, and 3 is tied
+// to 1. So the dag expression in the InstAlias definition specifies
+// only the un-tied operands 0,1,4 (aka $dest1,$dest2,$src), and
+// therefore that's also the set of indices we expect to see validated
+// by the checks in the generated printAliasInstr function.
+
+// CHECK: TestTargetInstPrinter::printAliasInstr
+// CHECK: MI->getOperand(0).isReg()
+// CHECK: MI->getOperand(1).isReg()
+// CHECK: MI->getOperand(4).isReg()


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53816.171494.patch
Type: text/x-patch
Size: 3480 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181029/2251d75b/attachment.bin>