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

[David Li via Phabricator via llvm-commits]

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


https://reviews.llvm.org/D36059