[PATCH] D71831: [PowerPC] Exploit the rldicl + rldicl when and with mask

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Sorry for the delayed comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4451
+  if (isUInt<16>(Imm64))
+    return false;
+
----------------
`andis.` in .td handles 0xABCD0000 u32imm, will this makes this kind imm worse?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4463
+    // +----------------------+     +----------------------+
+    //  0                    64      0                    64
+    // Left rotate ME + 1 bit first and then, mask with (MB + 63 - ME, 63),
----------------
s/64/63?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4467
+    Val = SDValue(CurDAG->getMachineNode(PPC::RLDICL, Loc, MVT::i64, Val,
+                                         getI64Imm(ME + 1, Loc),
+                                         getI64Imm((MB + 63 - ME) & 63, Loc)),
----------------
ME could be 63, putting 63 + 1 here seems unreasonable.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4474
+  }
+
+  // We can do special handling for pattern like this.
----------------
I personally think the logic for the special case is a little hard to follow.
I guess you want to convert the following imm: 0x00ffffffffff00ff,

can we first treat it as imm' 0xffffffffffff00ff? and we can call normal case like above, then we get
```
Val = SDValue(CurDAG->getMachineNode(PPC::RLDICL, Loc, MVT::i64, Val,
                                         getI64Imm(ME + 1, Loc),
                                         getI64Imm((MB + 63 - ME) & 63, Loc)),

SDValue Ops[] = {Val, getI64Imm(63 - ME, Loc), getI64Imm(0, Loc)};
CurDAG->SelectNodeTo(N, PPC::RLDICL, MVT::i64, Ops);
```
Notice that the second `rldicl` for above normal case has no clear operation, so its mb is 0,  can we add the clear for the highest 8 bits here?
Make a call for above normal case and then clear the highest bits in the second `rldicl`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4480
+  // +----------------------+
+  MB = countLeadingZeros(Imm64);
+  ME = 63 - countTrailingZeros(Imm64);
----------------
MB can be calculated after checking ME is valid?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4481
+  MB = countLeadingZeros(Imm64);
+  ME = 63 - countTrailingZeros(Imm64);
+  if (ME != 63)
----------------
check last bit is 1? Is it too complicated to call countTrailingZeros? 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4485
+
+  // Get the invert mask of [MB, ME].
+  uint64_t InvertMask =
----------------
ME is always 63? Why not use 63 directly?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4503
+  // +----------------------+    +----------------------+
+  //  0          32        64     0          32        64
+  // Rotate left MB2 + 1 bits and then, clear the bits (MB2, ME2)
----------------
s/32/31, s/64/63


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71831