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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:158
+  /// - OtherOpIdx - Other operand index
+  GIM_CheckTie,
+
----------------
qcolombet wrote:
> 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 :).
I see your point, I'll rename it. The reason I went with Tie was that the use of the opcode is triggered by using the same name multiple times like the MC layer.


================
Comment at: test/CodeGen/X86/GlobalISel/select-blsr.mir:15
+regBankSelected: true
+# CHECK:       registers:
+# CHECK-NEXT:     - { id: 0, class: gr32, preferred-register: '' }
----------------
qcolombet wrote:
> 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.
Ok


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:2091
+          InsnMatcher.addOperand(OpIdx++, Src->getName(), TempOpIdx);
+      if (!OM.isTied())
+        OM.addPredicate<LiteralIntOperandMatcher>(SrcIntInit->getValue());
----------------
qcolombet wrote:
> This is an optimization, not a correctness problem, right?
That's right. It's not wrong to check the original predicate again, it just saves on work.


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


https://reviews.llvm.org/D36618