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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Thank you for pointing me to this patch.

I've taken a look but other than a handful of nits this looks good.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:835
+  // {ones}{15-bit valve}
+  if ((64 - LZ) < 16 || (64 - LO) < 16) {
+    SDValue SDImm = CurDAG->getTargetConstant(Imm, dl, MVT::i64);
----------------
steven.zhang wrote:
> Is it more clear to use isInt<N>() ?
I agree. I think it would be clearer if you used `isInt<16>(Imm)`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:851
+  // i.e. The Imm can be presented as an int<32> value.
+  if (LZ > 32 || LO > 32) {
+    uint64_t ImmHi16 = (Imm >> 16) & 0xffff;
----------------
Same goes here:
`isInt<32>(Imm)`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1014
+    unsigned Opcode = ImmHi16 ? PPC::LIS8 : PPC::LI8;
+    Result = CurDAG->getMachineNode(Opcode, dl, MVT::i64, getI32Imm(ImmHi16));
+    Result = CurDAG->getMachineNode(PPC::ORI8, dl, MVT::i64, SDValue(Result, 0),
----------------
nit:
So if `ImmHi16 == 0`
we still add:
```
LI8  <reg>, 0
ORI8 <reg>, <reg>, (RotImm & 0xffff)
```
Why does it matter whether or not that first instruction is LIS8 or LI8? The two instructions will do the same thing if you feed them zero.




================
Comment at: llvm/test/CodeGen/PowerPC/unaligned-addressing-mode.ll:86
+; CHECK-NEXT:    #
+; CHECK-NEXT:    ldu r6, 8(r4)
 ; CHECK-NEXT:    ldx r7, r4, r5
----------------
Question:
Why does this test change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92089