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.
Details
Diff Detail
Event Timeline
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? |
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). |
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. |
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.
|
include/llvm/Analysis/AliasAnalysis.h | ||
---|---|---|
509 | I am not sure I understand. |
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. |
I agree, this seems like a useful generalization. 'RefModify' seems odd to me, however. We use ModRef for most related things, how about: