This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Merge rotate and clear into single instruction.
ClosedPublic

Authored by stefanp on Aug 18 2023, 6:11 PM.

Details

Summary

This patch tries to catch a codegen opportunity where the rotate and
mask can be merged into a single RLDCL instruction.

Diff Detail

Event Timeline

stefanp created this revision.Aug 18 2023, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 6:11 PM
stefanp requested review of this revision.Aug 18 2023, 6:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2023, 6:11 PM
stefanp added a reviewer: Restricted Project.Aug 18 2023, 6:24 PM
nemanjai added inline comments.Aug 20 2023, 4:31 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5102

Why does this need to be constant?

llvm/test/CodeGen/PowerPC/ppc-rotate-clear.ll
1

Please pre-commit the test case so this patch only shows the differences in code generation.

stefanp updated this revision to Diff 552121.Aug 21 2023, 1:39 PM

Comitted the test case first.
Rebased the change to top of main.

stefanp added inline comments.Aug 21 2023, 1:45 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5102

I'm trying to avoid matching the following kind of pattern:

%2 = tail call i64 @llvm.fshl.i64(i64 %word, i64 %word, i64 23)
%and1 = and i64 %2, 9223372036854775807

The reason for this is that in the above case we can't use RLDCL but we have to use RLDICL instead. See the function tworotates in the test case.

Now, I spent a bit of time and I realize that this condition won't trigger because the above test case is caught earlier by the check in tryBitPermutation. However, I still want to have this here just in case the above changes or the ordering changes in the future. It's just for safety to make sure that we don't try to use an immediate in a place where we shouldn't be using one.

lei added inline comments.Aug 30 2023, 8:31 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5097

nit: maybe move MB and dl def down to right before it's used on line 5106?

5631–5632

nit: indentation

amyk added inline comments.Aug 30 2023, 10:17 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
5102

Can we add that documentation in the code as to why want it to be a constant?

5105

Maybe we can save Val.getOperand(1) separately since we're using it twice in this function.

stefanp updated this revision to Diff 556024.Sep 6 2023, 6:36 AM

Added a comment.
Moved a couple of defs closer to where they are used.
Mostly just address remaining nits.

lei accepted this revision as: lei.Sep 6 2023, 9:30 AM

LGTM

This revision is now accepted and ready to land.Sep 6 2023, 9:30 AM
amyk accepted this revision as: amyk.Sep 6 2023, 10:05 AM

LGTM. Thanks for addressing my comment.

This revision was landed with ongoing or failed builds.Sep 7 2023, 6:25 AM
This revision was automatically updated to reflect the committed changes.