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

[EsmeYi via Phabricator via llvm-commits]

Esme added inline comments.


================
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);
----------------
stefanp wrote:
> 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)`.
Yes I agree with you, but it's hard to express all the patterns with `isInt<N>()`. In order to unify the expression of the patterns, I am very sorry that I did not modify them. I added some documentations and hope that's helpful. 


================
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),
----------------
stefanp wrote:
> 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.
> 
> 
Thanks for reviews! Yes, there is actually no difference between `LI 0` and `LIS 0`. I chose LI over LIS, just to avoid a fair amount of changes from `LI 0`  to `LIS 0` in the .ll files, since we preferred `LI 0` in the legacy code.


================
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
----------------
stefanp wrote:
> Question:
> Why does this test change?
Sorry I didn't notice this, I will have a look into it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92089