This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Future] Enable __builtin_mma_xxm[t|f]acc
ClosedPublic

Authored by kamaub on Jun 15 2023, 7:58 AM.

Details

Summary

Future cpu instructions dmxxinstdmr512 and dmxxextfdmr512 insert and extract
quad vectors from the new wide accumulator(wacc) register class.
The introduction of these new instructions renders the p10 instructions
xxmtacc and xxmfacc obsolete since the new wacc register class is a better
choice for handing quad vector operations. This patch ensures that, for
future cpu, instructions dmxxinstdmr512 and dmxxextfdmr512 are generated
by custom lowering the intrinsics for xxm[t|f]acc to produce no instructions.

Diff Detail

Event Timeline

kamaub created this revision.Jun 15 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2023, 7:58 AM
kamaub requested review of this revision.Jun 15 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2023, 7:58 AM
kamaub updated this revision to Diff 532240.Jun 16 2023, 11:04 AM

Updating wording.

kamaub retitled this revision from [PowerPC][Future] Enable __builtin_mma_xxm[t|f]acc on PFuture to [PowerPC][Future] Enable __builtin_mma_xxm[t|f]acc on future isa.Jun 16 2023, 11:09 AM
kamaub edited the summary of this revision. (Show Details)
kamaub updated this revision to Diff 532247.Jun 16 2023, 11:19 AM

Spelling mistake

amyk added a comment.Jun 16 2023, 11:39 AM

Should it be "Future ISA" instead of "Future isa"?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10751
10753

Avoiding the use of we here.

10754
10760

Just a question since I am curious, but is there any special reason on why 4 is picked for the SmallVector?

The SelectionDAG object may have an easier way for you do to what you are trying to do here. I've added a comment. If you use the selection DAG my nit will probably just disappear.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10759

There is an easier way to do this.
Take a look at:

DAG.ReplaceAllUsesWith(From, To);
10762

nit:
Full sentence for comment please. :)

kamaub updated this revision to Diff 533944.Jun 23 2023, 6:44 AM

Addressing review comments.

kamaub retitled this revision from [PowerPC][Future] Enable __builtin_mma_xxm[t|f]acc on future isa to [PowerPC][Future] Enable __builtin_mma_xxm[t|f]acc.Jun 23 2023, 6:44 AM
kamaub edited the summary of this revision. (Show Details)
kamaub edited the summary of this revision. (Show Details)
kamaub marked 6 inline comments as done.Jun 23 2023, 6:50 AM
lei added a subscriber: lei.Jun 23 2023, 7:16 AM
lei added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10746–10756

I find it confusing to start talking about new instructions when the block starts with the p10 intrinsics. I would add this comment to after handling of non-future ISA code. So after the if stmt and simplify it a bit as well.

llvm/test/CodeGen/PowerPC/mmaplus-intrinsics.ll
152

I realized these -O0 run line was not added as part of this patch, but I am wondering why we need to be explicitly running -O0 for these intrinsic tests. Are they generated differently for -O0 vs opt?

kamaub updated this revision to Diff 534006.Jun 23 2023, 10:26 AM

Addressing review comments.

kamaub edited the summary of this revision. (Show Details)Jun 23 2023, 10:27 AM
lei added inline comments.Jun 29 2023, 6:20 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10746–10747

nit:

kamaub added inline comments.Jun 30 2023, 2:29 PM
llvm/test/CodeGen/PowerPC/mmaplus-intrinsics.ll
152

I think the reason for adding O0 test cases was to make sure that FastISEL worked in these tested situations. For this patch specifically the xxm[t|f]acc intrinsics are not removed differently at O0, but having the check to make sure the the quad vector type is lowered to the expected dmr moves as a result of the intrinsic removal makes sense to me.

Even then, the amount of new O0 checks just for this change could be considered overkill.

Here I am adding a few new test cases but also moving some from some from mma-instrinsics.ll so i could move those new test to their own file where I do not need to test fastISEL, but I did not see a strong reasoning to do so at the time.

kamaub updated this revision to Diff 536468.Jun 30 2023, 3:45 PM
kamaub marked an inline comment as done.

Addressing a review comment.

kamaub marked an inline comment as done.Jun 30 2023, 3:45 PM
lei accepted this revision as: lei.Jul 12 2023, 1:21 PM

LGTM
Thx

This revision is now accepted and ready to land.Jul 12 2023, 1:21 PM
amyk accepted this revision as: amyk.Jul 14 2023, 8:41 AM

Also LGTM, thanks for answering my questions/addressing comments.

This revision was automatically updated to reflect the committed changes.