[PATCH] D10920: Unify isSafeToLoadUnconditionally and isDereferenceablePointer

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
Comment at: lib/Analysis/Loads.cpp:72
@@ -69,2 +71,3 @@
+                                       const TargetLibraryInfo *TLI) {
   // Zero alignment means that the load has the ABI alignment for the target
   if (Align == 0)
----------------
apilipenko wrote:
> hfinkel wrote:
> > Why can this part not be unified?
> > 
> > It seems this is the only difference. Why don't we just eliminate this function in favor of isDereferenceableAndAlignedPointer?
> > 
> isDereferenceableAndAlignedPointer checks properties of the pointer. isSafeToLoadUnconditionally calls isDereferenceableAndAlignedPointer and also checks whether this pointer was dereferenced in current BB. So, isSafeToLoadUnconditionally is more powerful and must be used everywhere to check if the load can be speculated.
I was unclear in my suggestion, rephrasing...

With this change isSafeToLoadUnconditionally, with ScanFrom == nullptr, is equivalent to isDereferenceableAndAlignedPointer. Thus, having these two functions separately exposed is not useful (and will only encourage people to use the less-powerful one directly out of ignorance).

Why don't you take the contents of isDereferenceableAndAlignedPointer and move it into this function. Or, move this function into ValueTracking. Either way, then replace all users of isDereferenceableAndAlignedPointer with isSafeToLoadUnconditionally (with a nullptr ScanFrom if necessary). You might take the existing isDereferenceableAndAlignedPointer and make it a static local function (instead of actually copying and pasting), which is probably better.

In the end, we should have one API function that other passes use, not two.


http://reviews.llvm.org/D10920