[PATCH] D20143: New pass: guard widening

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

In http://reviews.llvm.org/D20143#426695, @reames wrote:

> 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.


I'd say the pass in its current form is already fairly simple (~200 LOC, if you don't count the administrative overhead).  There //may// be more optimal ways of spreading out the logic across other passes, but before I do that I want to be clear about what works and what doesn't, from a performance standpoint.  Having a separate pass also lets me avoid making other passes pay the compile-time costs associated with widening (e.g. if I wanted to do cross-block widening in GVN/EarlyCSE, I'd have to compute the post-dom tree and loop info in GVN/EarlyCSE, something it doesn't do today).


================
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
+
----------------
reames wrote:
> Add a check not to show the second one being removed.
Will do.

================
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.
----------------
reames wrote:
> Did you mean trivial here?  An icmp is pretty easy to move around...
No, I did mean "non-trivial" (with "trivial" meaning "nothing needs to be moved"), but I see why that can sound weird.  I'll s/non-trivial/trivial/ :)

================
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.
----------------
reames wrote:
> Doing this here feels weird.  This should be an addition to loop unswitching or possibly even LICM itself.
This bit falls out of the cost/benefit logic we already have.  We can consider doing better / more nuanced version of widening in LICM and LoopUnswitch if needed.

================
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.
----------------
reames wrote:
> Note sure what you're getting at here.  The branch condition could still correlate.  Are you assuming healing?  If so, comment.
Yes, I've assumed healing (not yet implemented); will add a 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
----------------
reames wrote:
> Wait what?  A would absolutely expect cond_1 and cond_2 to be combined here.  Why would I ever want to not do that?
`cond_1` and `cond_2` are (intentionally) volatile loads, so you cannot hoist the computation producing `cond_2` to before the first guard (and hence can't widen the first one to check `cond_1`).


http://reviews.llvm.org/D20143