This is an archive of the discontinued LLVM Phabricator instance.

Unify isSafeToLoadUnconditionally and isDereferenceablePointer
AbandonedPublic

Authored by apilipenko on Jul 3 2015, 3:41 AM.

Details

Reviewers
hfinkel
Summary

There are several optimizations when we try to load speculatively. There are also two similar functions to determine whether it's a safe transformation:

  • isSafeToLoadUnconditionally
  • isDereferenceableAndAlignedPointer

Once we have D9791in place, we can unify these two functions. With this change isSafeToLoadUnconditionally calls isDereferenceableAndAlignedPointer to check whether speculative execution is possible.

As a side effect optimizations which use isSafeToLoadUnconditionally will benefit from dereferenceable attribute. Corresponding test cases added.

Diff Detail

Event Timeline

apilipenko updated this revision to Diff 29006.Jul 3 2015, 3:41 AM
apilipenko retitled this revision from to Unify isSafeToLoadUnconditionally and isDereferenceablePointer.
apilipenko updated this object.
apilipenko added a reviewer: hfinkel.
apilipenko added a subscriber: llvm-commits.

Since D9791 is landed this can go next. So, ping.

apilipenko updated this revision to Diff 34048.Sep 4 2015, 11:08 AM
apilipenko edited edge metadata.

Upload patch with full context.

hfinkel added inline comments.Sep 18 2015, 10:04 AM
lib/Analysis/Loads.cpp
72

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?

apilipenko added inline comments.Sep 21 2015, 8:40 AM
lib/Analysis/Loads.cpp
72

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.

hfinkel added inline comments.Dec 11 2015, 10:22 AM
lib/Analysis/Loads.cpp
72

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.

apilipenko updated this revision to Diff 44855.Jan 14 2016, 4:27 AM

Get rid of isDerferenceableAndAlignedPointer public API function. This revision depends on D16180.

reames added a subscriber: reames.Jan 14 2016, 5:25 PM

Artur I spoke this morning about ways to decompose this patch into easier to review chunks. I believe he plans to separate out at least two small patches to reduce the diff size. Specifically:

  1. One change to the order of arguments to isSafeToLoadUnconditionally. This is simply to reduce confusing diff lines and will go in directly without further review.
  2. One change to push down isDereferenceablePointer down into isSafeToLoadUnconditionally. This will remove most of the transform pass differences. This will need review, but should be quick to examine.

Once those two are in, this patch should become much simpler.