[PATCH] D31847: [PowerPC] Eliminate compares in instruction selection - Vol. 1

[Eric Christopher via Phabricator via llvm-commits]

echristo requested changes to this revision.
echristo added a comment.
This revision now requires changes to proceed.

Hi Nemanja!

The patch looks really good, my inline comments notwithstanding. I think they're mostly cosmetic rather than anything else with a little bit of refactoring.

Thanks!

-eric



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2497
+bool PPCDAGToDAGISel::tryEXTEND(SDNode *N) {
+  if (TM.getOptLevel() == CodeGenOpt::None || !TM.isPPC64())
+    return false;
----------------
The PPC64 bit is because of testing or some other reason?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2522
+
+  SDValue ConvOp = addExtOrTrunc(WideRes, Inputs32Bit, Output32Bit);
+  ReplaceNode(N, ConvOp.getNode());
----------------
I'm typically not a fan of boolean arguments to functions (for a number of reasons). Can we refactor this to deal with not doing that? Either pass N down, or an enum with one of the 4 states?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2535-2541
+  bool IsTruncateOfSext = Opc == ISD::TRUNCATE &&
+    (Input.getOperand(0).getOpcode() == ISD::AssertSext ||
+     Input.getOperand(0).getOpcode() == ISD::SIGN_EXTEND);
+  bool IsSextLoad = InputLoad && InputLoad->getExtensionType() == ISD::SEXTLOAD;
+  if (InputConst || IsTruncateOfSext || Opc == ISD::AssertSext ||
+      Opc == ISD::SIGN_EXTEND_INREG || Opc == ISD::SIGN_EXTEND || IsSextLoad)
+    return Input;
----------------
This could probably use some more detailed comments.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2539
+  bool IsSextLoad = InputLoad && InputLoad->getExtensionType() == ISD::SEXTLOAD;
+  if (InputConst || IsTruncateOfSext || Opc == ISD::AssertSext ||
+      Opc == ISD::SIGN_EXTEND_INREG || Opc == ISD::SIGN_EXTEND || IsSextLoad)
----------------
Feels like these could be separate and separately documented early returns based on the above booleans/variables?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2559-2560
+  // to conservatively actually clear the high bits.
+  if (InputZExtConst || Opc == ISD::AssertZext ||
+      Opc == ISD::ZERO_EXTEND || NonSextLoad)
+    return Input;
----------------
Feels like these could be separate and separately documented early returns based on the above booleans/variables?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2570-2571
+// Handle a 32-bit value in a 64-bit register and vice-versa.
+SDValue PPCDAGToDAGISel::addExtOrTrunc(SDValue NatWidthRes, bool From32Bit,
+                                       bool To32Bit) {
+  SDLoc dl(NatWidthRes);
----------------
See my earlier comment on boolean variables. I'm pretty sure it'll help readability here as well.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2574
+  SDValue ConvOp = NatWidthRes;
+  if (From32Bit && !To32Bit) {
+    SDValue ImDef(CurDAG->getMachineNode(PPC::IMPLICIT_DEF, dl, MVT::i64), 0);
----------------
I find it handy to describe the code you expect to return for these blocks.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2597
+  default: return SDValue();
+  case ISD::SETEQ: {
+    SDValue Xor = IsRHSZero ? LHS :
----------------
Document each case with the return instructions that you're going to be emitting? (and other places)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2639-2640
+/// values is a power of two while the other is zero.
+SDValue PPCDAGToDAGISel::getSETCCInGPR(SDValue Compare, bool IsSext,
+                                       bool InvertCC) {
+  assert((Compare.getOpcode() == ISD::SETCC ||
----------------
Ditto with the boolean arguments?


Repository:
  rL LLVM

https://reviews.llvm.org/D31847