[PATCH] D13764: [LoopUnswitch] Spill unswitch quota based on branch weight

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:187
@@ +186,3 @@
+    // to split evenly.
+    uint64_t DefaultOldLoopWeight = 50;
+    uint64_t DefaultNewLoopWeight = 50;
----------------
This should probably be a cl::opt.  One should be 100-TheOther.

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:573
@@ +572,3 @@
+        // is not available.
+        uint64_t NewLoopWeight = DefaultNewLoopWeight,
+                 OldLoopWeight = DefaultOldLoopWeight;
----------------
This isn't quite right when the LoopCond isn't BI->getCondition.  The branch's weight doesn't tell us anything about LoopConds likelyhood of failing.  

================
Comment at: lib/Transforms/Scalar/LoopUnswitch.cpp:580
@@ +579,3 @@
+
+              ConstantInt *CI = mdconst::extract<ConstantInt>(MD->getOperand(1));
+              NewLoopWeight = CI->getValue().getZExtValue();
----------------
Wait, what?!  Oh, these aren't normalized.  I'd probably do the normalization here rather than at the use site.  It'd be more clear.  

================
Comment at: test/Transforms/LoopUnswitch/unswitch-with-branch-weight.ll:18
@@ +17,3 @@
+;; This can be unswitched twice based on cond1 and cond2, ending with 4
+;; loops if we have enough quota. However, if we dont have enough quota,
+;; we should spend most of them on the hot branch based on branch weight.
----------------
This comment (both here and in the change description) is really confusing.  You're not choosing between cond1 and cond2 at all.  You're choosing which *version* of cond2 might get unswitched once the unswitch of cond1 is done.  

================
Comment at: test/Transforms/LoopUnswitch/unswitch-with-branch-weight.ll:45
@@ +44,3 @@
+
+; CHECK:      call void @dummy1() #0
+; CHECK-NEXT: call void @dummy3() #0
----------------
This test case is really hard to follow.  Please write this out in full so that the unswitched loop structure can be seen.


http://reviews.llvm.org/D13764