[PATCH] D31851: [PowerPC] Eliminate compares - add handling for logical operations without the use of condition registers

[Eric Christopher via Phabricator via llvm-commits]

echristo added a comment.

A few inline comments, a couple of refactorings and some cleanup, otherwise looks OK.

One other comment: This is starting to feel like we're doing all of isel in C++ in the backend. I'd like to look at being able to do as much of this in generic code as possible. Perhaps take a look and see what we can manage that way and a comment at some point before all of this in the source?

Thanks!



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2511
 
+static bool isLogicOp(unsigned Opc) {
+  return Opc == ISD::AND || Opc == ISD::OR || Opc == ISD::XOR;
----------------
Comment?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2569
+// achieved with xor %a, -1).
+SDValue PPCDAGToDAGISel::getLogicOpInGPR(SDValue LogicOp, bool KeepInGPR) {
+  assert(isLogicOp(LogicOp.getOpcode()) &&
----------------
bool operand :)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2578
+  // Special case: xor %a, -1
+  bool IsNot = isBitwiseNot(LogicOp);
+
----------------
The naming convention here is a little hard to parse later on. I'm seeing IsNot as !a and not a ^ -1...



================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2609
+  // a bitwise negation operation.
+  if (!Op1 || (!Op2 && !IsNot))
+    return SDValue();
----------------
Maybe call them LHS and RHS? :)


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2662
+bool PPCDAGToDAGISel::tryLogicOpOfCompares(SDNode *N) {
+  if (TM.getOptLevel() == CodeGenOpt::None || !TM.isPPC64())
+    return false;
----------------
Needs the same commentary as the previous patch.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2814
 
+static bool allUsesExtend(SDValue Compare) {
+  if (Compare.hasOneUse())
----------------
Description?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:2850
+  // of the i1 result (i.e. uses that require the result in the CR).
+  if ((Compare.getOpcode() == ISD::SETCC) && !allUsesExtend(Compare))
+    return SDValue();
----------------
This should probably be an earlier return before you do the work above?


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3207
+      return;
+  }
   case ISD::ADD: {
----------------
fallthrough?


Repository:
  rL LLVM

https://reviews.llvm.org/D31851