[PATCH] D20143: New pass: guard widening

[Philip Reames via llvm-commits]

reames added a comment.

A few initial comments mostly looking at the test cases.  I'll take a look at the actual code separately.

My meta concern here is that I think you're trying to be way too aggressive here.  "One pass to rule them all" is not generally a good approach.  I suspect that smaller changes to InstCombine, EarlyCSE, GVN, and LICM would end up covering most of the interesting cases.  Even if we decide to go with a single check widening pass, I'd prefer to see it start dead simple and evolve.


================
Comment at: test/Transforms/GuardWidening/basic.ll:26
@@ +25,3 @@
+; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 %wide.chk) [ "deopt"() ]
+; CHECK:  br i1 undef, label %left, label %right
+
----------------
Add a check not to show the second one being removed.

================
Comment at: test/Transforms/GuardWidening/basic.ll:42
@@ +41,3 @@
+
+; Like @f_1, but we have some non-trivial code we need to hoist before
+; we can widen a dominanting check.
----------------
Did you mean trivial here?  An icmp is pretty easy to move around...

================
Comment at: test/Transforms/GuardWidening/basic.ll:97
@@ +96,3 @@
+; But hoisting out of control flow is fine if it makes a loop computed
+; condition loop invariant.  This behavior may require some tuning in
+; the future.
----------------
Doing this here feels weird.  This should be an addition to loop unswitching or possibly even LICM itself.

================
Comment at: test/Transforms/GuardWidening/basic.ll:121
@@ +120,3 @@
+
+; Hoisting out of control flow is also fine if we can widen the
+; dominating check without doing any extra work.
----------------
Note sure what you're getting at here.  The branch condition could still correlate.  Are you assuming healing?  If so, comment.

================
Comment at: test/Transforms/GuardWidening/basic.ll:166
@@ +165,3 @@
+; CHECK:  %cond_1 = load volatile i1, i1* %cond_buf
+; CHECK:  %cond_3 = icmp ult i32 %a, 7
+; CHECK:  %wide.chk = and i1 %cond_1, %cond_3
----------------
Wait what?  A would absolutely expect cond_1 and cond_2 to be combined here.  Why would I ever want to not do that?


http://reviews.llvm.org/D20143