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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

Thanks

In https://reviews.llvm.org/D36086#844494, @rovka wrote:

> 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).


I was surprised to find out it was an operator in the TreePatternNode. For predicated immediates like `simm8:$src`, TreePatternNode translates it to `(imm:i32)<<P:Predicate_simm8>>:$imm` with both the predicate and the name attached to the `imm` operator rather than the value inside it. I'd always thought `imm` and similar were leaves. The tablegen classes are also called ImmLeaf/PatLeaf which reinforced that. It only makes sense when you see that ImmLeaf/PatLeaf are subclasses of PatFrag and that ConstantSDNode holds the immediate internally instead of referencing another SDNode.

I've handled it the same way because some predicates belong on the operator (particularly load/store predicates). As a result, sinking it to the operand would require special casing the immediate case and handling it separately from other PatFrags. Also, at the point the predicates are handled, the relevant OperandMatcher hasn't been created yet so it's just easier to apply the predicate where it was found.

For the 'No predicates comment', making it specify which kind of predicates might help a little but it's doesn't fully remove the confusion.

> 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.

I agree. What it says is correct but it fails to mention important details. I'll update this comment.



================
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;
----------------
rovka wrote:
> You forgot == G_CONSTANT :)
Again? :-) I must have copy/pasted from the last one and forgot to fix this one too


================
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
----------------
rovka wrote:
> Maybe you should also assert that this is different from GIPFP_Invalid.
Ok


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1000
 
+/// Generates code to check that an operand meets an immediate predicate.
+class InstructionImmPredicateMatcher : public InstructionPredicateMatcher {
----------------
This should mention that it's an instruction predicate and why.


https://reviews.llvm.org/D36086