As a first step in the direction of assumed invariant loads (loads that are not written in some context) we now detect and hoist definitively invariant loads. Invariant loads will be separated from their statements and preloaded in the code generation. The preloaded value is then used in the new SCoP. If the load is only conditionally executed the preloaded version will also only be executed under the same condition, hence we will never access memory that wouldn't have been accessed otherwise. This is also the most distinguishing feature compared to licm.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Some comments to indicate things I will address.
include/polly/ScopInfo.h | ||
---|---|---|
875 ↗ | (On Diff #35811) | replace "the whole SCoP." by "this statement." |
1151 ↗ | (On Diff #35811) | Will add something here. |
1156 ↗ | (On Diff #35811) | This should be only in the next commit, the same for the "required ones" part in the brief desc. |
lib/Analysis/ScopInfo.cpp | ||
2297 ↗ | (On Diff #35811) | This is only needed for the next patch. D13195 |
lib/CodeGen/BlockGenerators.cpp | ||
1 ↗ | (On Diff #35811) | My bad. |
lib/CodeGen/IslNodeBuilder.cpp | ||
822 ↗ | (On Diff #35811) | left over. |
What's the motivational use case that cannot be handled by LICM?
Could you create a separate pass that modifies the ScopInfo, i.e. lib/Transform/HoistInvariantLoads.cpp
Michael
include/polly/CodeGen/IslNodeBuilder.h | ||
---|---|---|
209 ↗ | (On Diff #35811) | Grammar? I don't understand this sentence |
lib/Analysis/ScopInfo.cpp | ||
1299 ↗ | (On Diff #35811) | list::remove_if |
2285 ↗ | (On Diff #35811) | Could you refactor removing a ScopStmt from a Scop into another function so it might be used by other code as well? |
lib/CodeGen/IslNodeBuilder.cpp | ||
830 ↗ | (On Diff #35811) | Can you write a comment about how the CFG will look like? |
I forgot to split the test cases, sorry. Here is one:
also note the commit message (it is stated there).
Could you create a separate pass that modifies the ScopInfo, i.e. lib/Transform/HoistInvariantLoads.cpp
I am unsure (not if I could but if it makes sense). This patch performs "licm" only as an optimization, true, but from D13195 onwards we will need it as a crucial part of the SCoP generation. Hence, I think it belongs (at least in the long run) in the ScoP class.
include/polly/CodeGen/IslNodeBuilder.h | ||
---|---|---|
209 ↗ | (On Diff #35811) | I'll simplify the sentence and add a comma. |
lib/Analysis/ScopInfo.cpp | ||
1299 ↗ | (On Diff #35811) | I am unsure. What should I replace by list::remove_if and why? |
2285 ↗ | (On Diff #35811) | We can do that easily as soon as another usecase presents itself. If ppl insist we can do it now but I don't see the benefit at the moment. |
lib/CodeGen/IslNodeBuilder.cpp | ||
830 ↗ | (On Diff #35811) | I could, but it would just be the CFG from here a couple of times in sequence. |
lnt shows no problems, no gains and <10 regressions that are more than noise. However, these regressions sum up to less than 0.5s in total. Hence the detection of invariant loads costs a little bit but not much.
OK
lib/Analysis/ScopInfo.cpp | ||
---|---|---|
1299 ↗ | (On Diff #35811) | Does this just remove all elements of MAL from MemAccs? Maybe if not by remove_if (std::std::set_difference), this could be probably rewritten using STL functions. It also looks fragile because it depends on the order of elements in MAL and MemAccs. Please add at least a comment about what the code is supposed to do. |
2238 ↗ | (On Diff #35811) | These do not initialize the Scop, but transform it. |
lib/CodeGen/IslNodeBuilder.cpp | ||
830 ↗ | (On Diff #35811) | Linking into files in Phabricator reviews doesn't work for me. It just jumps to a location before it lazily loads the diffs. You could just refer to that code location in the comment. |