<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 1, 2017 at 12:53 AM, Chandler Carruth via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">chandlerc added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: lib/Transforms/Scalar/<wbr>FastPathLibCalls.cpp:90<br>
+  /// Checks whether a value is known non-zero at a particular location.<br>
+  bool isKnownNonZero(Value *V, Instruction &I) {<br>
+    // If value tracking knows enough, we're done.<br>
----------------<br>
</span><div><div class="h5">davidxl wrote:<br>
> chandlerc wrote:<br>
> > davidxl wrote:<br>
> > > chandlerc wrote:<br>
> > > > davidxl wrote:<br>
> > > > > Is it better to rely on instcombine and cfg simplification to get rid of the redundant zero guard which can be more general?<br>
> > > > Sadly, they're *less* powerful than this. You'd need PRE or something similar to get it, powered by PredicateInfo and GVN. Maybe JumpThreading and LVI could get it?<br>
> > > ><br>
> > > > All of these seem really heavy weight to run this late in the pipeline. =/<br>
> > > Cases like the following can be handled by -instcombine + -simplfycfg.   Wrapping the second redundant test into another check also works fine.<br>
> > ><br>
> > > What are the interesting cases that can not be handled?<br>
> > ><br>
> > > efine void @set1_nonzero1(i8* %ptr, i64 %size) {<br>
> > > ; CHECK-LABEL: define void @set1_nonzero1(<br>
> > > entry:<br>
> > >   %zero_cond = icmp eq i64 %size, 0<br>
> > >   br i1 %zero_cond, label %exit, label %test<br>
> > ><br>
> > > test:<br>
> > >   %nonzero_cond = icmp ne i64 %size, 0<br>
> > >   br i1 %nonzero_cond, label %call, label %exit<br>
> > ><br>
> > > call:<br>
> > >   call void @llvm.memset.p0i8.i64(i8* %ptr, i8 15, i64 %size, i32 1, i1 false)<br>
> > >   ret void<br>
> > ><br>
> > > exit:<br>
> > >   ret void<br>
> > > }<br>
> > ><br>
> > > declare void @llvm.memset.p0i8.i64(i8* writeonly, i8, i64, i32, i1)<br>
> > I'm honestly surprised instcombine+simplify-cfg would get this. It seems outside of their purview to do this kind of predicate analysis... I couldn't find where it does this in a cursory glance through the code.<br>
> ><br>
> > Anyways, we don't run instcombine after this pass, but only inst simplify. Not sure we want to add that expensive of a pass when this code can just handle the specific cases it wants.<br>
> instcombine just converts the second predicate into true/false.<br>
><br>
> However, it seems simplifycfg alone can clean it up.    This pass can potentially be moved ahead to share some of the common cleanups?<br>
</div></div>I'm not sure how it converts the second predicate into a constant in the hard cases though.<br>
<br>
<br>
Anyways, see my response to Craig regarding simplify-cfg -- it's doing a *much* more limited form of this. I can add a test case that shows this at least.<br></blockquote><div><br></div><div>Fine.  Longer term, the code that does redundant predicate elimination here should perhaps be extracted and put into some common passes. We should probably also file bugs tracking such missing opportunities. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
As for moving this pass to share cleanups, see my comments to Mehdi about the challenges of changing the position of the pass. I don't see a great way to get it in front of instcombine, but maybe there is one.<br></blockquote><div><br></div><div>Ok.   I am fine with the self contained approach for now. Perhaps introducing a flag to make the behavior off by default for now so that folks have more time to test it out?    Or if size is the only concern, we can turn this on for O3.</div><div><br></div><div>David</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D36059" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36059</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>