[PATCH] D72250: [NFC][PowerPC] Refactor the tryAndWithMask()

[Amy Kwan via Phabricator via llvm-commits]

amyk added a comment.

Just some minor comments, I think it overall LGTM but am curious in knowing any comments from others.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4370
   SDValue Val = N->getOperand(0);
-  unsigned Imm, Imm2, SH, MB, ME;
-  uint64_t Imm64;
-
+  unsigned SH, MB, ME;
   // If this is an and of a value rotated between 0 and 31 bits and then and'd
----------------
Just curious, but any reason on why we split up the declaration of variables? I mean since the original had `Imm, SH, MB, ME` declared together?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4384
+  if (isRunOfOnes(Imm, MB, ME) && Val.getOpcode() != ISD::ROTL) {
+    SDValue Ops[] = {Val, getI32Imm(0, dl), getI32Imm(MB, dl),
+                     getI32Imm(ME, dl)};
----------------
Unnecessary whitespace change between `{ }` (also seen in other places for `SDValue Ops[]` brackets as well)? 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4413
+    // We can only do it if the MB is larger than 32 and MB <= ME
+    // as RLWINM will replace the content of [0 - 32) with [32 - 64) even
+    // we didn't rotate it.
----------------
I think it would be more clear if it is read, "`. . . replace the contents of [0 - 32) with [32 - 64) even if we didn't rotate it.` 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4750
   case ISD::AND:
-    // If this is an 'and' with a mask, try to emit rlwinm/rldicl/rldicr
-    if (tryAndWithMask(N))
+    if (tryAsSingleRLWINM(N) || tryAsSingleRLWIMI(N) || tryAsSingleRLDICL(N) ||
+        tryAsSingleRLDICR(N) || tryAsSingleRLWINM8(N))
----------------
I think it would still be valuable to have a comment here to document this case with `AND`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72250