[PATCH] D93300: [PowerPC] Exploit paddi instruction on Power 10 for constant materialization

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1030
+  unsigned TO = countTrailingOnes<uint64_t>(Imm);
+  unsigned FO = countLeadingOnes<uint64_t>(Imm << LZ);
+  unsigned Hi32 = Hi_32(Imm);
----------------
NeHuang wrote:
> why do we name it as FO instead of LO here? can you please explain what FO stands for? 
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>
       ^------^
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1095
+  if ((LZ + FO + TO) > 30) {
+    APInt SignedInt34 = APInt(34, (Imm >> TO) & 0x3ffffffff);
+    APInt Extended = SignedInt34.sext(64);
----------------
NeHuang wrote:
> 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));
> ```
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.


================
Comment at: llvm/test/CodeGen/PowerPC/p10-constants.ll:13
 ; CHECK-LABEL: t_16BitsMinRequiring34Bits:
-; CHECK:	pli r3, 32768
-; CHECK-NEXT:	blr
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pli r3, 32768
----------------
steven.zhang wrote:
> I guess you miss to rebase the patch, so we see this irrelative change ?
You are right. I will rebase this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93300/new/

https://reviews.llvm.org/D93300