[PATCH] D72250: [NFC][PowerPC] Refactor the tryAndWithMask()
Amy Kwan via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 17 15:49:31 PST 2020
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
More information about the llvm-commits
mailing list