This is an archive of the discontinued LLVM Phabricator instance.

Sink store based on alias analysis
ClosedPublic

Authored by ellab on Nov 26 2014, 5:51 AM.

Details

Summary

Similarly to patch D5991, I use alias analysis to define whether the given instruction is a barrier for store sinking.
For 2 identical stores, following instructions are checked in the both basic blocks, to determine whether they are sinking barriers.

Diff Detail

Event Timeline

ellab updated this revision to Diff 16648.Nov 26 2014, 5:51 AM
ellab retitled this revision from to Sink store based on alias analysis.
ellab updated this object.
ellab edited the test plan for this revision. (Show Details)
ellab added reviewers: Gerolf, delena, hfinkel.
ellab set the repository for this revision to rL LLVM.
ellab added subscribers: hfinkel, Unknown Object (MLST).
hfinkel added inline comments.Nov 27 2014, 6:43 PM
include/llvm/Analysis/AliasAnalysis.h
8

I agree, this seems like a useful generalization. 'RefModify' seems odd to me, however. We use ModRef for most related things, how about:

canInstructionRangeModRef
lib/Transforms/IPO/ArgumentPromotion.cpp
2

Indentation seems off here.

lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
-2

We know that Inst is a store, so this should be cast, not dyn_cast.

-5–1

Why did you add the Inst->isUsedOutsideOfBlock check. We know that Inst is a store, and stores can't be 'used' anywhere.

4

I think we prefer the logical operator before the line break, not after. Can you move the first '&&' to the line above?

ellab updated this revision to Diff 16766.Dec 1 2014, 7:52 AM

Applied hfinkel's comments

Gerolf added inline comments.Dec 1 2014, 11:44 AM
include/llvm/Analysis/AliasAnalysis.h
519

mode -> Mode

lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
148

Rename to SI

430

You could pass Loc as the third parameter and save one call to getLocation.

test/Transforms/InstMerge/st_sink_no_barrier_call.ll
46

Please remove

ellab updated this revision to Diff 16852.Dec 2 2014, 11:35 PM

Applied Gerolf's comments

ellab added inline comments.Dec 2 2014, 11:59 PM
test/Transforms/InstMerge/st_sink_no_barrier_call.ll
46

If I remove the attribute, the call to foo will be a barrier (as in the test above - st_sink_barrier_call).

reames added a subscriber: reames.Dec 3 2014, 9:57 AM
reames added inline comments.
include/llvm/Analysis/AliasAnalysis.h
509

It would be good to keep this interface consistent with the one you're changing. Leaving wrappers for the old interface names would be fine, or converting the BasicBlock versions to the ModRef form. I'd slightly prefer leaving convenience wrappers under the old names.

lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
421–432

Rather than stopping on the first readwrite call, this function will now rescan the end of the basic block for each store in the basic block. That seems mildly expensive: O(N^2) in the number of instructions in the BB. Have you considered the implications for compile time?

424

Use cast.

delena added inline comments.Dec 4 2014, 4:04 AM
lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
421–432

Hi Philip,

We already discussed about this when I implemented load hoisting. When you go through the first basic block, you don't know what to check. Because not every store/load is a barrier.

We are looking at pointer location while going through the second block. If the both strore instructions are indentical we start to look for a barrier - we do less checks in comparison to the previous implementation.
I do not expect the implications for compile time in this case.

  • Elena
ellab updated this revision to Diff 17030.Dec 8 2014, 12:59 AM

changed the cast syntax

ellab added inline comments.Dec 8 2014, 1:09 AM
include/llvm/Analysis/AliasAnalysis.h
509

I am not sure I understand.
I left the names of canBasicBlockModify as is.
However, I changed the name of the canInstructionRangeModRef convenience wrapper as it is no longer only modify (I now noticed the comment was not changed - will update now).
Do you think it should have been done somehow otherwise?

ellab updated this revision to Diff 17033.Dec 8 2014, 1:56 AM

Fixed a comment

Gerolf edited edge metadata.Dec 8 2014, 11:33 AM

Other than nits it LGTM. Thanks for your nice work!

include/llvm/Analysis/AliasAnalysis.h
515

mode -> Mode (capital letter)

515

You could clean up the legacy common from "the value pointed to by Ptr" to "the Location" (or \param Location)

525

It should be Mode (capital M).

lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
424

There is similar legacy code you might want to clean up too, And you could use auto.

ellab updated this revision to Diff 17270.Dec 14 2014, 10:48 PM
ellab edited edge metadata.

Applied Gerolf's comments

delena accepted this revision.Oct 8 2015, 12:19 AM
delena edited edge metadata.

This revision is committed. Turned to "Accept" in order to close.

This revision is now accepted and ready to land.Oct 8 2015, 12:19 AM
delena closed this revision.Oct 8 2015, 12:19 AM