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

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

Please upload the patch with full context, see: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

As a general comment, I'd make this a bit more general. The core problem, as you know, if that the logic that takes care of eliminating unnecessary round-tripping through crbit regs is in PPCISelLowering, and thus is BB local. Using crbits makes a lot of sense only when some input comes from a comparison, or feeds a conditional branch. If none of the i1 values comes from a comparison, and none feeds a branch, then it seems likely we'll have unnecessary round-tripping through crbit regs.

Thus, it seems like you could start with all i1 consumers that are not selects or conditional branches (returns, function arguments, [zs]exts), and build a tree of defines all the way back to the initial producers. Unless one of the producers is a comparison, then promote everything to i32.


================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:42
@@ +41,3 @@
+  SetVector<Value *> findAllDefs(Value *V) {
+    SetVector<Value *> defs;
+    SetVector<Value *> workList;
----------------
Why are you using SetVector here? We normally use SmallVector and SmallPtrSet for worklist iterations. Also, you can use pop_back_val below.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:75
@@ +74,3 @@
+
+    assert(false && "Unexpected value");
+  }
----------------
Use llvm_unreachable.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:93
@@ +92,3 @@
+      if (!isa<PHINode>(V) && !isa<Constant>(V) && !isa<ReturnInst>(V))
+	return false;
+
----------------
There is a lot of weird indentation in this file (tabs?). Please fit it. Running clang-format over the file is the easiest way.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:97
@@ +96,3 @@
+      for (auto &W : V->uses())
+	if (!defs.count(W))
+	  return false;
----------------
You probably need to ignore debug intrinsics here (at least).

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:100
@@ +99,3 @@
+
+    DenseMap<Value *, Value *> boolToIntMap;
+    for (auto &V : defs)
----------------
Local variables in LLVM start with a capital letter.

================
Comment at: lib/Transforms/Scalar/BoolRetToInt.cpp:118
@@ +117,3 @@
+
+  void getAnalysisUsage(AnalysisUsage &) const {}
+};
----------------
This needs to be filled in appropriately. I assume it can have at least:

  AU.addPreserved<DominatorTreeWrapperPass>();


================
Comment at: test/Transforms/BoolRetToInt/BoolRetToIntTest.ll:7
@@ +6,3 @@
+; Function Attrs: nounwind readnone
+; CHECK: i32 @notBoolRet()
+define signext i32 @notBoolRet() #0 {
----------------
Use CHECK-LABEL for the function-name matching

================
Comment at: test/Transforms/BoolRetToInt/BoolRetToIntTest.ll:15
@@ +14,3 @@
+; Function Attrs: nounwind
+; CHECK: i1 @find
+define zeroext i1 @find(i8** readonly %begin, i8** readnone %end, i1 (i8*)* nocapture %hasProp) #1 {
----------------
CHECK-LABEL

================
Comment at: test/Transforms/BoolRetToInt/BoolRetToIntTest.ll:48
@@ +47,3 @@
+
+attributes #0 = { nounwind readnone "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="ppc64le" "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+power8-vector,+vsx,-qpx" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="ppc64le" "target-features"="+altivec,+bpermd,+crypto,+direct-move,+extdiv,+power8-vector,+vsx,-qpx" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
Please remove unnecessary attributes and metadata.


http://reviews.llvm.org/D14064