[PATCH] D36086: [globalisel][tablegen] Add support for ImmLeaf without SDNodeXForm

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

This should have a summary.

I'm probably just bikeshedding a bit, but why is this an instruction predicate and not an operand predicate? I understand that you know it always refers to the second operand of the G_CONSTANT, but I think it looks a bit strange e.g. in your test you have a "No predicates" comment on the MIs[0] Operand 1 which looks a bit surprising (I had a bit of an "ooooh, of course, because it's an instruction predicate" moment reading that test). It's especially weird considering that the Matcher has this comment: "Generates code to check that an operand meets an immediate predicate." - I'd expect it to be an operand predicate then.



================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelector.h:96
   GIM_CheckNumOperands,
+  /// Check an immediate predicate on the specified truction
+  /// - InsnID - Instruction ID
----------------
typo (truction)


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:135
+      assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+      assert(State.MIs[InsnID]->getOpcode() && "Expected G_CONSTANT");
+      int64_t Value = 0;
----------------
You forgot == G_CONSTANT :)


================
Comment at: include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:131
+      int64_t InsnID = MatchTable[CurrentIdx++];
+      int64_t Predicate = MatchTable[CurrentIdx++];
+      DEBUG(dbgs() << CurrentIdx << ": GIM_CheckImmPredicate(MIs[" << InsnID
----------------
Maybe you should also assert that this is different from GIPFP_Invalid.


https://reviews.llvm.org/D36086