[TableGen:AsmWriter] Cope with consecutive tied operands.

[simon_tatham]

Summary

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.

[rengolin]

Hi Simon,

This makes sense to me, and it's nice to see tablegen tests, but what is the real effect of this patch?

As you say in the description, it allows variants of the affected instructions (like UMLAL), but it does not specify which variants, nor it shows, via tests, how those would now be matched with the changes.

Can you add some tests for the affected instructions?

cheers,
--renato

[simon_tatham]

Can you add some tests for the affected instructions?

I'm afraid the actual case where I ran into this bug is not yet in-tree.

Diffing all the generated AsmWriter.inc files before and after this change, it looks as if the only alias that's affected in any in-tree target is vtrans2x2 in the Hexagon backend, and even that's only in principle, because that backend doesn't enable alias disassembly in any case (it doesn't #define PRINT_ALIAS_INSTR before including HexagonGenAsmWriter.inc).

I thought it would be helpful to get the bug fix into Tablegen itself as soon as possible in case anyone else's out-of-tree or in-development work is affected. But if you'd prefer to leave it unfixed until an end-to-end test case is available, then I can come back to this later.

[rengolin]

Right, I don't see anything wrong with it, but I'm adding other back-end maintainers and Jakob, TableGen's maintainer, so we're sure this is as harmless as I seem to think it is. :)

[simon_tatham]

Could I give this a gentle ping, please?

If you'd like a concrete example of how it could affect a real back end, I've just come up with a trivial example using an existing Thumb instruction. Suppose you added this declaration to the bottom of lib/Target/ARM/ARMInstrThumb2.td:

def DemoAddSquareAlias : t2InstAlias<"demoaddsquare${p} $d1, $d2, $s", (t2UMLAL rGPR:$d1, rGPR:$d2, rGPR:$s, rGPR:$s, pred:$p), 1>;

which defines "demoaddsquare d1,d2,s" to add s^2 to the 64-bit value in registers d1,d2, by expanding to a special case of UMLAL in which both source registers are the same, and sets the priority so that the alias should be used in disassembly.

Without this patch, the following command (in which the encoding is for umlal r0, r1, r2, r2):

llvm-mc -triple thumbv7a -disassemble <<<"[0xe2,0xfb,0x02,0x01]"

recognises the instruction as a case of this demo alias, and gets as far as printing the alias mnemonic (demoaddsquare), but then fails an assertion:

MCInst.h:77: int64_t llvm::MCOperand::getImm() const: Assertion `isImm() && "This is not an immediate"' failed.

But with the patch, that disassembly succeeds, and prints demoaddsquare r0, r1, r2 just as it should.

[rengolin]

Right, that's all very hypothetical, but the change is harmless otherwise and generally an improvement of the code. I can't see any impact this would have in code generation or compilation time, so LGTM.

Sorry it took so long.

test/TableGen/ConsecutiveTiedAlias.td

+// 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()

utils/TableGen/AsmWriterEmitter.cpp

for (auto &Alias : Aliases.second) {
       IAP.addCond(Cond);
 
       bool CantHandle = false;
 
       unsigned MIOpNum = 0;
       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;
+        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())
+                Operands[TiedOpNum].Rec->getName()) {
               ++MIOpNum;
+              continue;
+            }
+          }
+          break;
         }
 
         std::string Op = "MI->getOperand(" + utostr(MIOpNum) + ")";
 
         const CodeGenInstAlias::ResultOperand &RO = CGA.ResultOperands[i];
 
         switch (RO.Kind) {
         case CodeGenInstAlias::ResultOperand::K_Record: {