[PATCH] D131296: [PowerPC] Add support for extending and truncating values

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:247
+             .addImm(PPC::sub_32);
+    constrainSelectedInstRegOperands(*MI, TII, TRI, RBI);
+
----------------
What will happen if we don't constrain the middle result, like `ImpDefReg` and result of `INSERT_SUBREG`? I assume the register class setting in the source code here should match what `IMPLICIT_DEF` `INSERT_SUBREG` and the final `RLDICL` requires? 


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:254
+  } else {
+    assert(MRI.getType(SrcReg).getSizeInBits() < 32 && "Unexpected src size!");
+    MI = BuildMI(MBB, I, I.getDebugLoc(), TII.get(PPC::RLDICL), DstReg)
----------------
Do we need to handle any arbitrary bit here, like i7/i9? I assume the illegal types should be already handled in legalizer pass?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:258
+             .addImm(0)
+             .addImm(64 - MRI.getType(SrcReg).getSizeInBits());
+  }
----------------
hmm, what if the register class of `SrcReg` is not `g8rc`? Directly putting it as operand of `PPC::RLDICL` seems not right to me. For example if the input is `gprc`, constraining may change it to `g8rc` by adding a `COPY`? but that `COPY` will not tell how to handle the high 32 bit?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:284
+  assert(MRI.getType(DstReg).getSizeInBits() == 64 &&
+         "Unexpected size of source operand");
+
----------------
nit: sounds like here should be `dest operand`?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:292
+  case 16:
+    Opc = PPC::EXTSH8;
+    break;
----------------
We may need to change the tb for the 8 and 16 bits in another patch. For example add patterns for `EXTSB8_32_64` and `EXTSH8_32_64`?


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:336
+    return selectCopy(I, TII, MRI, TRI, RBI);
+  } else if (DstSz == 32 && SrcSz == 64) {
+    I.setDesc(TII.get(TargetOpcode::COPY));
----------------
Weird that `trunc i64 to i32` can not be handled in table gen?
There is pattern:
```
def : Pat<(i32 (trunc i64:$in)),
          (EXTRACT_SUBREG $in, sub_32)>;
```

Seems the patterns in the match table are all about trunc from i64/i32 -> i1. Maybe a closely look is needed to understand why for later improvement.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCInstructionSelector.cpp:342
+    MachineInstr *MI =
+        BuildMI(MBB, I, I.getDebugLoc(), TII.get(PPC::RLDICL), DstReg)
+            .addReg(SrcReg)
----------------
Same as sext, directly putting SrcReg as operand of PPC::RLDICL will cause copy like `%1:gprc = COPY %3:g8rc` and this COPY can not be expanded in later pseudo expansion.


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

https://reviews.llvm.org/D131296