[PATCH] D14064: Convert Returned Constant i1 Values to i32 on PPC64

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

I think that we should first add this pass into the PPC backend; there maybe other backends that can use it, but we should only generalize when we have a use case. There are other IR-level passes completely defined in the backend (PPCLoopPreIncPrep and PPCLoopDataPrefetch for example). Emulate how those are added and don't touch the common files/directories here.


================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:32
@@ +31,3 @@
+// in data flow using SelectInsts. Selects are slow on some
+// architectures (PPC), so this probably isn't good in general, but
+// for the special case of i1, the Selects could be further lowered to
----------------
Saying PPC here is over-generalizing, because it is not true on the A2, or on various Freescale cores. You specifically mean P7/P8.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:85
@@ +84,3 @@
+
+    
+    if (PHINode *P = dyn_cast<PHINode>(V)) {
----------------
Remove extra blank line.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:127
@@ +126,3 @@
+
+    SmallVector<const PHINode *, 8> toRemove;
+    for (const auto &P : Promotable) {
----------------
toRemove -> ToRemove

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:129
@@ +128,3 @@
+    for (const auto &P : Promotable) {
+
+      // Condition 2
----------------
Remove blank line.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:135
@@ +134,3 @@
+          toRemove.push_back(P);
+
+      // Condition 3
----------------
Please extract this code into a function (or lambda), so that you can do an early return here so that you don't do any further processing on PHIs you've already decided to remove.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:139
@@ +138,3 @@
+        if (!isa<Constant>(Op) && !isa<Argument>(Op) &&
+	    !isa<CallInst>(Op) && !isa<PHINode>(Op))
+          toRemove.push_back(P);
----------------
Indentation odd here.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:150
@@ +149,3 @@
+      for (const auto &P : Promotable) {
+
+        // Condition 4
----------------
Remove blank line.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:156
@@ +155,3 @@
+              toRemove.push_back(P);
+
+        // Condition 5
----------------
Same here; there's no need to check condition 5 if condition 4 fails. Refactor so you don't.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:184
@@ +183,3 @@
+            Changed |=
+	      runOnUse(R->getOperandUse(0), PromotablePHINodes, Bool2IntMap);
+
----------------
Indentation is odd here.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:191
@@ +190,3 @@
+      }
+    }
+
----------------
We should also start searching from [sz]ext i1 -> iN nodes. Either do that, or add a note.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:210
@@ +209,3 @@
+      if (!isa<PHINode>(V) && !isa<Constant>(V) &&
+	  !isa<Argument>(V) && !isa<CallInst>(V))
+        return false;
----------------
Indentation.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:224
@@ +223,3 @@
+      if (!BoolToIntMap.count(V))
+	BoolToIntMap[V] = translate(V);
+
----------------
Indentation.


http://reviews.llvm.org/D14064