[PATCH] D36059: [memops] Add a new pass to inject fast-path code for specific library function calls.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/FastPathLibCalls.cpp:90
+  /// Checks whether a value is known non-zero at a particular location.
+  bool isKnownNonZero(Value *V, Instruction &I) {
+    // If value tracking knows enough, we're done.
----------------
davidxl wrote:
> chandlerc wrote:
> > davidxl wrote:
> > > chandlerc wrote:
> > > > davidxl wrote:
> > > > > Is it better to rely on instcombine and cfg simplification to get rid of the redundant zero guard which can be more general?
> > > > 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?
> > > > 
> > > > All of these seem really heavy weight to run this late in the pipeline. =/
> > > Cases like the following can be handled by -instcombine + -simplfycfg.   Wrapping the second redundant test into another check also works fine.
> > > 
> > > What are the interesting cases that can not be handled?
> > > 
> > > efine void @set1_nonzero1(i8* %ptr, i64 %size) {
> > > ; CHECK-LABEL: define void @set1_nonzero1(
> > > entry:
> > >   %zero_cond = icmp eq i64 %size, 0
> > >   br i1 %zero_cond, label %exit, label %test
> > > 
> > > test:
> > >   %nonzero_cond = icmp ne i64 %size, 0
> > >   br i1 %nonzero_cond, label %call, label %exit
> > > 
> > > call:
> > >   call void @llvm.memset.p0i8.i64(i8* %ptr, i8 15, i64 %size, i32 1, i1 false)
> > >   ret void
> > > 
> > > exit:
> > >   ret void
> > > }
> > > 
> > > declare void @llvm.memset.p0i8.i64(i8* writeonly, i8, i64, i32, i1)
> > 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.
> > 
> > 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.
> instcombine just converts the second predicate into true/false.
> 
> However, it seems simplifycfg alone can clean it up.    This pass can potentially be moved ahead to share some of the common cleanups?
I'm not sure how it converts the second predicate into a constant in the hard cases though.


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.


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.


https://reviews.llvm.org/D36059