This is an archive of the discontinued LLVM Phabricator instance.

[globalisel][tablegen] Add support for fpimm and import of APInt/APFloat based ImmLeaf.
ClosedPublic

Authored by dsanders on Aug 10 2017, 1:37 AM.

Details

Summary

There's only a tablegen testcase for IntImmLeaf and not a CodeGen one
because the relevant rules are rejected for other reasons at the moment.
On AArch64, it's because there's an SDNodeXForm attached to the operand.
On X86, it's because the rule either emits multiple instructions or has
another predicate using PatFrag which cannot easily be supported at the
same time.

Event Timeline

dsanders created this revision.Aug 10 2017, 1:37 AM
dsanders updated this revision to Diff 118419.Oct 10 2017, 10:40 AM

Rebase and ping

qcolombet accepted this revision.Oct 12 2017, 11:27 AM

LGTM with nitpicks.

Also, when you commit, I would do the AArch64 and X86 changes in different commits.

lib/Target/AArch64/AArch64InstructionSelector.cpp
709

I didn't get that comment.
Could you elaborate?

utils/TableGen/CodeGenDAGPatterns.h
485

Could we move these in GlobalISelEmitter?
Something like GlobalISelEmitter::getPredicateOpcode(TreePredicateFn)?

495

Ditto.

This revision is now accepted and ready to land.Oct 12 2017, 11:27 AM

Thanks for the reviews

lib/Target/AArch64/AArch64InstructionSelector.cpp
709

This patch causes the rules involving fpimm0 to be imported so we no longer need to handle that case in the C++. It's not harmful to leave it in, but doing so means we can't tell if the test case passes because tablegen works or because the C++ caught it. The comment is intended to explain why we choose not to handle 0.0 in the C++ code even though it would be valid.

utils/TableGen/CodeGenDAGPatterns.h
485

Ok

495

Ok

qcolombet added inline comments.Oct 12 2017, 2:16 PM
lib/Target/AArch64/AArch64InstructionSelector.cpp
709

Make sense.
Say all that in the comment then :).

dsanders marked 5 inline comments as done.Oct 13 2017, 2:11 PM
dsanders added inline comments.
utils/TableGen/CodeGenDAGPatterns.h
495

Looking at this one a bit closer, I'm not sure I should move it. It makes use of other private members (immCodeUsesAPInt and immCodeUsesAPFloat) so I'd have to make those public to be able to move it. I don't think those other members should be public since they describe properties of getImmCode() which is also private.

Would it be ok with you to leave this one here?

qcolombet added inline comments.Oct 13 2017, 2:22 PM
utils/TableGen/CodeGenDAGPatterns.h
495

Yep that's fine.
This one doesn't say GISel ;).

dsanders added inline comments.Oct 13 2017, 2:26 PM
utils/TableGen/CodeGenDAGPatterns.h
495

Thanks

dsanders closed this revision.Oct 13 2017, 2:28 PM
tstellar added inline comments.
test/TableGen/GlobalISelEmitter.td
770

Is there a reason why GIR_CopyFConstantAsFPImm was not implemented in InstructionSelector?

dsanders added inline comments.Apr 22 2018, 10:36 PM
test/TableGen/GlobalISelEmitter.td
770

The most likely explanation is that there were other issues with importing any of the rules that would have made use of it at the time. Do you have a test case that's hitting this?

tstellar added inline comments.Apr 25 2018, 7:25 AM
test/TableGen/GlobalISelEmitter.td
770

Yes, D45994 hits this. I created D45990 to try to implement GIR_CopyFConstantAsFPImm. The main problem is that the AMDGPUGenGlobalISel.inc won't build, because this enum value is not defined. The pattern that this was generated for, we don't need and won't ever use, so it's not a problem if this remains unimplemented, we just need to be able to build AMDGPUGenGlobalISel.inc .