[PATCH] D36618: [globalisel][tablegen] Simplify named operand/operator lookups and fix a wrong-code bug this revealed.

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Looks mostly good.
Couple of comments below.



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:158
+  /// - OtherOpIdx - Other operand index
+  GIM_CheckTie,
+
----------------
Food for thought: I wonder if we should use a different name like CheckIsSame, because when I see tie, it makes me think of the tie operand in the MCDesc and that's not what this is.
I believe I won't be the only one confused by this at first :).


================
Comment at: test/CodeGen/X86/GlobalISel/select-blsr.mir:15
+regBankSelected: true
+# CHECK:       registers:
+# CHECK-NEXT:     - { id: 0, class: gr32, preferred-register: '' }
----------------
Could you add a comment explaining what this test is checking, beyond blsr selection?
In particular, I would have expected something about the fact that the operands of the both G_ADD and G_AND should be the same.

Could you also add a negative test case?
In particular, one that would have match with the bug but does now. I expect you could just change the parameters of one of G_ADD or G_ADD.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2091
+          InsnMatcher.addOperand(OpIdx++, Src->getName(), TempOpIdx);
+      if (!OM.isTied())
+        OM.addPredicate<LiteralIntOperandMatcher>(SrcIntInit->getValue());
----------------
This is an optimization, not a correctness problem, right?


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2092
+      if (!OM.isTied())
+        OM.addPredicate<LiteralIntOperandMatcher>(SrcIntInit->getValue());
     } else
----------------
Could we have this check pushed into ::addPredicate?
That way we wouldn't have to spread it all around.


https://reviews.llvm.org/D36618