[PATCH] D92089: [PowerPC] Materialize i64 constants by enumerated patterns.

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

Some coding style comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:803
 
+/* For any 32 < Num < 64, check if the Imm contains at least Num consecutive
+   zeros.
----------------
Please use C++ comment style: //


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:809
+*/
+static bool findContiguousZeros(uint64_t Imm, unsigned Num, unsigned &Shift) {
+  assert((Num > 32 && Num < 64) && "Unexpected number.");
----------------
It makes more sense to me to have int findContiguousZerosAtLeast(Imm, Num). And you can check it like this:

if ((Shift=findContiguousZerosAtLeast(Imm, 49)) || (Shift = ...))


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:820
+
+static SDNode *selectI64ImmByPattern(SelectionDAG *CurDAG, const SDLoc &dl,
+                                     uint64_t Imm, unsigned &InstCnt) {
----------------
The ByPattern here is a bit confusing. How about selectSimpleI64Imm() ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:837
+
+  /* Following patterns use 1 instructions to materialize the big constant */
+  InstCnt = 1;
----------------
Use C++ comment style.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:859
+  if (LZ > 32 || LO > 32) {
+    uint64_t LiImm = (Imm >> 16) & 0xffff;
+    unsigned LiOpcode = LiImm ? PPC::LIS8 : PPC::LI8;
----------------
ImmHi16 ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:860
+    uint64_t LiImm = (Imm >> 16) & 0xffff;
+    unsigned LiOpcode = LiImm ? PPC::LIS8 : PPC::LI8;
+    Result = CurDAG->getMachineNode(LiOpcode, dl, MVT::i64, getI32Imm(LiImm));
----------------
Opcode is ok as you don't know if it is LIS or LI.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:861
+    unsigned LiOpcode = LiImm ? PPC::LIS8 : PPC::LI8;
+    Result = CurDAG->getMachineNode(LiOpcode, dl, MVT::i64, getI32Imm(LiImm));
+    return CurDAG->getMachineNode(PPC::ORI8, dl, MVT::i64, SDValue(Result, 0),
----------------
The logic here is not quite right. If the High 16 bits of the Imm is zero, you still produce the LI which is not needed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:879
+  if ((LZ + TO) > 48) {
+    assert(LZ < 48 && "Unexpected shift value.");
+    Result = CurDAG->getMachineNode(PPC::LI8, dl, MVT::i64,
----------------
How about if LZ == 48 ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:880
+    assert(LZ < 48 && "Unexpected shift value.");
+    Result = CurDAG->getMachineNode(PPC::LI8, dl, MVT::i64,
+                                    getI32Imm((Imm >> (48 - LZ) & 0xffff)));
----------------
Some comments on bit pattern changed during these two instructions are needed to help understand what you are trying to do.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:888
+  if ((LZ + FO + TO) > 48) {
+    Result = CurDAG->getMachineNode(PPC::LI8, dl, MVT::i64,
+                                    getI32Imm((Imm >> TO) & 0xffff));
----------------
Again, you need some comments to indicate that, you are trying to leverage the sign extend of LI to make them as ones, then, rotate and clear it.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:893
+  }
+  // {32 zeros}{****}{15-bit value}
+  if (LZ == 32 && ((Lo32 & 0x8000) == 0)) {
----------------
The implementation is not align with the comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:903
+      findContiguousZeros(~Imm, 49, Shift)) {
+    uint64_t RotImm = (Imm >> Shift) | (Imm << (64 - Shift));
+    Result = CurDAG->getMachineNode(PPC::LI8, dl, MVT::i64,
----------------
There is rotate routines for you to do this.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:983
+  // No more than 3 instructions.
+  SDNode *Result = selectI64ImmByPattern(CurDAG, dl, Imm, InstCntDirect);
+  if (InstCnt)
----------------
It is more clean to have code like this:
```
if (SDNode *Result = ...) {
  If (InstCnt)
   *InstCnt = ...
  return Result;
}
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:998
+                                    SDValue(Result, 0), getI32Imm(Hi16));
+    InstCntDirect += 1;
+  }
----------------
++InstCntDirect


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2059
+      unsigned NumSelectInsts = 0;
+      selectI64Imm(CurDAG, dl, Mask, &NumSelectInsts);
       if (Use32BitInsts)
----------------
Add the assertion here to make sure that, the return value from selectI64Imm is not nullptr.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2288
       } else {
-        if (InstCnt) *InstCnt += selectI64ImmInstrCount(Mask) + /* and */ 1;
-
-        SDValue MaskVal = SDValue(selectI64Imm(CurDAG, dl, Mask), 0);
-        Res =
-          SDValue(CurDAG->getMachineNode(PPC::AND8, dl, MVT::i64,
-                                         ExtendToInt64(Res, dl), MaskVal), 0);
+        unsigned NumSelectInsts = 0;
+        SDValue MaskVal =
----------------
NumOfInstrs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92089