Page MenuHomePhabricator

[PowerPC] Exploit paddi instruction on Power 10 for constant materialization
ClosedPublic

Authored by stefanp on Dec 15 2020, 7:33 AM.

Details

Summary

Starting with Power 10 the instruction paddi is available to use.
The instruction allows for immediates that are 34 bits.

This patch adds exploitation of the paddi instruction to allow us
to materialize constants.

Diff Detail

Event Timeline

stefanp created this revision.Dec 15 2020, 7:33 AM
stefanp requested review of this revision.Dec 15 2020, 7:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 7:33 AM
stefanp added reviewers: NeHuang, Restricted Project.Dec 15 2020, 7:36 AM
steven.zhang added inline comments.
llvm/test/CodeGen/PowerPC/p10-constants.ll
13

I guess you miss to rebase the patch, so we see this irrelative change ?

NeHuang added inline comments.Jan 6 2021, 11:36 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1030

why do we name it as FO instead of LO here? can you please explain what FO stands for?

1095

Can we merge the code for the three patterns? Maybe we can do something like:
Var = TZ/30-LZ/TO for different scenarios and use Var below (feel free to change Var to other name):

APInt SignedInt34 = APInt(34, (Imm >> (Var)) & 0x3ffffffff);
APInt Extended = SignedInt34.sext(64);
Result = CurDAG->getMachineNode(PPC::PLI8, dl, MVT::i64,
                                getI64Imm(*Extended.getRawData()));
return CurDAG->getMachineNode(PPC::RLDICL, dl, MVT::i64, SDValue(Result, 0),
                              getI32Imm(Var), getI32Imm(LZ));
stefanp added inline comments.Feb 3 2021, 4:30 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1030

The FO is for Following Ones. It is the number of ones that follow the first zeros.
The idea is that you are counting these:

00...0011...1110<bits>
       ^------^
1095

That's a good idea. I'm going to try to merge these into one and see how that looks.
I want to try to keep the comments to explain each scenario.

llvm/test/CodeGen/PowerPC/p10-constants.ll
13

You are right. I will rebase this patch.

stefanp added inline comments.Feb 3 2021, 12:05 PM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1095

I realized now why this isn't going to work. I thought it was going to work when you mentioned it but now I see why they are separate.
The instructions are not identical. When the shift is TZ the instruction is RLDIC and the other two are RLDICL. I don't think it is worth changing how the code is written to merge just two of the patterns (it will make it more confusing when looking at why all the patterns have their own if statement and then two of them are merged) Therefore I'm just going to leave the code as-is.

stefanp updated this revision to Diff 321186.Feb 3 2021, 12:17 PM

Rebased the patch to the top of main.

Tried to merge three of the cases but it looks like that won't work as one of
the cases actually uses a different instrution (I didn't even notice this)
RLDIC vs. RLDICL. I don't want to merge the remaining two as I think this is
more clear.

Gentle ping.

amyk added a subscriber: amyk.Feb 17 2021, 2:58 PM

Just a few minor nit comments.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1068

Minor spelling mistake.

llvm/test/CodeGen/PowerPC/p10-constants.ll
208

I noticed we don't really use the term Trailing Ones, but we use Following Ones. Would it be better if we said Following Ones here?

219

Just double checking but does this test correspond to the (LZ + FO + TO) > 30 case?

If that is the case, should the comment be Leading Zeros + Following Ones + Trailing Zeros?

amyk added inline comments.Feb 18 2021, 11:49 AM
llvm/test/CodeGen/PowerPC/p10-constants.ll
208

Actually, you can disregard this comment. I made a mistake. We have both Trailing Ones and Following Ones in this patch. Sorry about that!

stefanp updated this revision to Diff 324720.Feb 18 2021, 11:51 AM

Fixed spelling mistake.

lei added inline comments.Mar 9 2021, 6:47 AM
llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
1034–1036

I see this lambda defined in at least 2 other functions in this file. Maybe it's time to make this an actual function rather then a lambda within the different functions. But that can be a followup NFC patch.

lei accepted this revision as: lei.Mar 9 2021, 7:06 AM
This revision is now accepted and ready to land.Mar 9 2021, 7:06 AM
amyk accepted this revision as: amyk.Mar 10 2021, 7:20 AM

Sorry for the delay and thank you for addressing my previous comments. I think this LGTM.

stefanp updated this revision to Diff 329905.Mar 11 2021, 3:47 AM

Rebased patch to prepare for commit.

This revision was landed with ongoing or failed builds.Mar 11 2021, 6:38 AM
This revision was automatically updated to reflect the committed changes.
hokein added a subscriber: hokein.Mar 12 2021, 4:55 AM

hi @stefanp, this patch caused an asan error when running on ninja check-llvm-codegen-powerpc.

llvm-project/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1046:48: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')

hokein added a subscriber: RKSimon.Mar 12 2021, 5:53 AM

hi @stefanp, this patch caused an asan error when running on ninja check-llvm-codegen-powerpc.

llvm-project/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1046:48: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')

ok, looks like @RKSimon fixed them in https://github.com/llvm/llvm-project/commit/f6524b4ada823a9766f7cbdfc16e052971e005f6, Thanks!