[PATCH] D9151: Loop Versioning for LICM

[Nema, Ashutosh via llvm-commits]

>> Can I keep this like vectorizer ?

> No, but how about we start with something simple. Start with allowing only 
> functions for which AA.doesNotAccessMemory(CS) returns true. Then we 
> can add more sophistication later as desired. I don't want to hold this up 
> unnecessarily, but tying this check to vectorizable functions is just not the 
> right thing.

Sure Hal, will modify like this.

Thanks,
Ashutosh

-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov] 
Sent: Tuesday, October 27, 2015 10:59 PM
To: Nema, Ashutosh
Cc: javed absar; llvm-commits at lists.llvm.org; silviu baranga; reviews+D9151+public+ac7c33357b316d74 at reviews.llvm.org; anemet at apple.com; listmail at philipreames.com; charlie turner
Subject: Re: [PATCH] D9151: Loop Versioning for LICM

----- Original Message -----
> From: "Ashutosh via llvm-commits Nema" <llvm-commits at lists.llvm.org>
> To: reviews+D9151+public+ac7c33357b316d74 at reviews.llvm.org, anemet at apple.com, listmail at philipreames.com, "charlie
> turner" <charlie.turner at arm.com>
> Cc: "javed absar" <javed.absar at arm.com>, llvm-commits at lists.llvm.org, "silviu baranga" <silviu.baranga at arm.com>
> Sent: Tuesday, October 6, 2015 11:02:14 PM
> Subject: RE: RE: [PATCH] D9151: Loop Versioning for LICM
> 	
> 
> 
> Hal, is below comments on call handling looks OK to you ?
> 
> 
> > Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:356
> 
> > @@ +355,3 @@
> 
> > + const bool IsAnnotatedParallel = CurLoop->isAnnotatedParallel();
> 
> > + // We dont support call instructions. however, we ignore few
> > intrinsic
> 
> > + // and libfunc callsite. We don't allow non-intrinsic,
> > non-libfunc callsite
> 
> > ----------------
> 
> > Why not? I understand that the vectorizer has these checks, but I
> > don't see why this belongs in an LICM pass?
> 
> > 
> 
> > There is a possibility that call may modify aliasing behavior,
> > which may defeat the purpose of versioning & runtime checks.
> 
> 
> Ah, good point. However, please then replace this check with an
> appropriate AA getModRef-type check for whether the call might alias
> with the relevant pointers (if the function, for example, does not
> alias with any of the loop-invariant accesses, then it can't affect
> what you're trying to do, although you'll need to be somewhat more
> careful about adding the noalias metadata to those functions, etc.).
> 
> I’m not sure that itself would be sufficient, consider indirect calls
> probably need to consider function pointer in runtime check.
> Also there is a possibility that in call argument one of the runtime
> pointer escaped, in such cases it become more difficult to ensure
> correctness.
> That’s why for simplicity I considered vectorizer approach to
> consider only few functions.
> 
> Can I keep this like vectorizer ?

No, but how about we start with something simple. Start with allowing only functions for which AA.doesNotAccessMemory(CS) returns true. Then we can add more sophistication later as desired. I don't want to hold this up unnecessarily, but tying this check to vectorizable functions is just not the right thing.

Thanks again,
Hal

> 
> Regards,
> Ashutosh
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory