Page MenuHomePhabricator

[globalisel][tablegen] Add support for ImmLeaf without SDNodeXForm
ClosedPublic

Authored by dsanders on Jul 31 2017, 5:30 AM.

Details

Summary

This patch adds support for predicates on imm nodes but only for ImmLeaf and not for PatLeaf or PatFrag and only where the value does not need to be transformed before being rendered into the instruction.

The limitation on PatLeaf/PatFrag/SDNodeXForm is due to differences in the necessary target-supplied C++ for GlobalISel.

Depends on D36085

Event Timeline

dsanders created this revision.Jul 31 2017, 5:30 AM
rovka edited edge metadata.Aug 17 2017, 9:16 AM

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.

include/llvm/CodeGen/GlobalISel/InstructionSelector.h
96

typo (truction)

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
137

Maybe you should also assert that this is different from GIPFP_Invalid.

141

You forgot == G_CONSTANT :)

Thanks

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.

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
137

Ok

141

Again? :-) I must have copy/pasted from the last one and forgot to fix this one too

utils/TableGen/GlobalISelEmitter.cpp
1020

This should mention that it's an instruction predicate and why.

dsanders edited the summary of this revision. (Show Details)Aug 18 2017, 7:34 AM
dsanders updated this revision to Diff 112185.Aug 22 2017, 9:25 AM
dsanders marked 6 inline comments as done.

Fixed the nits and added some additional tests I noticed were missing from test/TableGen/GlobalISelEmitter.td

rovka accepted this revision.Aug 22 2017, 7:07 PM

Thanks for the updates, this looks really good now!

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
143

Supernit: Predicate != GIPFP_Invalid seems more direct and readable...

This revision is now accepted and ready to land.Aug 22 2017, 7:07 PM
dsanders added inline comments.Aug 23 2017, 3:16 AM
include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
143

Thanks for spotting this. The 0 /* GIPFP_Invalid */ bit was an intermediate step caused by a circular dependency problem. I fixed that in this update but it looks like I didn't correct this bit afterwards. I'll correct this in the commit

The reason for Predicate > GIPFP_Invalid is that the table consists of signed integers and valid predicates are all positive non-zeros.

dsanders closed this revision.Aug 23 2017, 5:15 AM