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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

I think, this patch overrall looks great as it simplify our logic of the instruction selection for i64 imm. Please update the check with isS[U]Int<>() to make the code more clear. And I assume that, you have run the bmk and bootstrap with this patch.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:803
 
+bool findContiguousZeros(uint64_t Imm, unsigned Num, unsigned &Shift) {
+  unsigned Hi_tz = countTrailingZeros<uint32_t>(Hi_32(Imm));
----------------
It should be static at least. And please add necessary documentation on the semantics for this function.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:813
+
+static SDNode *selectI64ImmByPattern(SelectionDAG *CurDAG, const SDLoc &dl,
+                                     uint64_t Imm, unsigned *InstCnt) {
----------------
I prefer to use reference for the InstrCnt and if for the case that we don't need it, just pass a dummy cnt.


================
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);
----------------
Is it more clear to use isInt<N>() ?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:841
+  // {ones}{15-bit valve}{16 zeros}
+  if (TZ > 15 && (LZ > 32 || LO > 32))
+    return CurDAG->getMachineNode(PPC::LIS8, dl, MVT::i64,
----------------
Is it more clear to use: TZ > 15 && isInt<16>(Imm >> 16) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92089