Page MenuHomePhabricator

[ARM,MVE] Rename and clean up VCTP IR intrinsics.
ClosedPublic

Authored by simon_tatham on Fri, Nov 22, 3:47 AM.

Details

Summary

D65884 added a set of Arm IR intrinsics for the MVE VCTP instruction,
to use in tail predication. But the 64-bit one doesn't work properly:
its predicate type is <2 x i1> / v2i1, which isn't a legal MVE
type (due to not having a full set of instructions that manipulate it
usefully). The test of vctp64 in basic-tail-pred.ll goes through
opt fine, as the test expects, but if you then feed it to llc it
causes a type legality failure at isel time.

The usual workaround we've been using in the rest of the MVE
intrinsics family is to bodge v2i1 into v4i1. So I've adjusted the
vctp64 IR intrinsic to do that, and completely removed the code (and
test) that uses that intrinsic for 64-bit tail predication. That will
allow me to add isel rules (upcoming in D70485) that actually generate
the VCTP64 instruction.

Also renamed all four of these IR intrinsics so that they have mve
in the name, since its absence was confusing.

Diff Detail

Event Timeline

simon_tatham created this revision.Fri, Nov 22, 3:47 AM
This revision is now accepted and ready to land.Fri, Nov 22, 7:31 AM

I agree that this is the best way forward. Any 2 wide masked vectors loads/stores would be expanded, as far as I understand, so the MVETailPredication pass would never handle those cases (although I'm not sure that is explicit in the pass anywhere), and we would never generate them.

@samparker you happy with removing arm_vctp64? Maybe one to try and get working in the future if it makes sense.

samparker added inline comments.Mon, Dec 2, 6:55 AM
llvm/include/llvm/IR/IntrinsicsARM.td
780

I think we should just remove this definition for now, unless you've already got some clang tests that generate the IR?

simon_tatham marked an inline comment as done.Mon, Dec 2, 6:57 AM
simon_tatham added inline comments.
llvm/include/llvm/IR/IntrinsicsARM.td
780

I have – those are in the next patch in the stack, D70485. I'm about to add the vctp64q ACLE intrinsic which will expand to that IR intrinsic.

samparker added inline comments.Mon, Dec 2, 7:47 AM
llvm/include/llvm/IR/IntrinsicsARM.td
780

Ok. Fine by me then.

This revision was automatically updated to reflect the committed changes.