This pass adds a new pass that reduces the number of assumes.
this pass will remove redondont knowledge, cleanup dead assumes and merge assumes in the same basic block when possible.
Details
- Reviewers
jdoerfert
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This is cool, thanks! We might soon have all we need :)
Do we have a test with argument attributes that make assumptions obsolete?
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
---|---|---|
136–137 | Commit this as nfc or merge it into the commit that made it unsused or use Intr below. | |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
233 | Why Value* and how can it be null? | |
251 | Can we user eraseFromParent here instead of the two step process? Think about early continue in the loop (Begin == End). The (!I) can be folded into the next condition. | |
266 | There is no knowledge in ignore tags, right? I guess we could add a continue to not rely on the one 3 lines below. | |
277 | What is updated here? I first thought the argument attribute is merged into the assume but I cannot figure out how the value is passed. I guess a lambda makes this clear (see also below). On second thought, I doubt we can update the llvm.assume, so if this is what you are doing, I doubt it is legal. It works for "bit-wise" attribues like nonnull but not for "stateful" ones like dereferenceable. Thinking this further, we need to distinguish these "stateful" attributes in other places as well. The attributor performs a similar lookup (mistake) right now (I think). | |
287 | Same as with the update above. | |
301 | Can we make these lambdas? -goto remove +{ Remove(BOI); continue; } Arg is probably not the best name for the Use* above. | |
316 | Why can we move assumptions up to the first PHI? | |
330 | What happens to the conditions (operand 0)? | |
381 | We need comments on these methods explaining what they do. |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
---|---|---|
233 | in a past version i was iterating over the assumption cache, this can now be simplified. | |
277 | the update code changed in the last update. what is getting updated is the other knowledge either the argument of the function or the other assume. this only occurs when there isValidAssumeForContext returns true in both direction, so the assume are in equivalent contexts. | |
316 | when generating a new assume this code tries to put that assume as close as possible to the beginning of the BasicBlock. such that isValidAssumeForContext Queries on that assume can be resolved with a simple comesBefore instead of a loop of isGuaranteedToTransferExecutionToSuccessor. at this point we know that all assumes from Begin to End can be merged because there is no instruction between them for which isGuaranteedToTransferExecutionToSuccessor doesn't return true, so if the first assume is reached all assumes in the range will be. in the first loop, InsertPt is adjusted to keep the assume dominated by all values it uses. in the second loop, if the insertion is still above Begin after the first loop we look how high can it be and place it there. | |
330 | i am not sure what you mean. | |
381 | i added comments on functions. hope its enought |
More comments. Did you run this on the test suite with the new PM and knowledge retention enabled? If it passes there I think we can merge this after this set of comments is through.
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
---|---|---|
230 | "ignore" should be a constexpr value in some header and everyone should use it. | |
247 | I think the second sentence is somehow broken. | |
291 | I'm always confused by the "context instruction" thing. Does it mean before or after that instruction? I mean, if &*F.getEntryBlock().getFirstInsertionPt() may not return is this doing the right thing? Do we have a test? | |
308 | I'm a little afraid this is non-deterministic. It probably depends on the AC->assumptions and the containers/maps you use. WDYT? | |
316 | I failed to see how the second loop adjusts the position. The comment helps, thanks! | |
330 | You seem to only check if the WasOn part of the bundle comes before the current insertion point and not the condition (operand 0) of the assume. We need to make sure we don't move: %cond = ... call llvm.assume(%cond) [align(%arg, 32)] | |
390 | ++SplitIt |
on a separate note.
i started testing on real-word code with enable-knowledge-retention.
after a few fixes the compiler didn't crashed on any application i tried. and there doesn't seem to be any miss-compile either.
however the compile time cost is very high, originally around +15% after some tweaking i lowered it to 9%.
from what i saw the cause of the slowdown is cause by salvageKnowledge that generate lots of extra instructions and many part of the optimizer scale on the number of instruction.
here is the time taken by the parts i changed/added in the tweaked version:
0.43% in salvageKnowledge
0.01% in assumes Queries
0.70% in the assume simplify pass (i added it in 4 places in the pipeline as part of tweaks)
although they could probably be optimized, they are not a big part of the slowdown.
i still have a few idea to try out and there is also lots of arbitrary decision i made during development that could be guided by measurement.
so the current overhead can surely be lowered but i don't think i can get it to a reasonable level without significantly limiting the preserved information.
i didn't start measuring the runtime impact but i don't expect it to be good yet. since droppable uses are not widely in place.
any thought ?
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
---|---|---|
230 | i added IgnoreBundleTag but sadly some uses of "ignore" in IR can't be replaced by it. since it resides in Analysis | |
291 | i don't know either but this is covered by test4A and it seems to be before the context instruction or the test would fail. the test verifies that we do not replace the assume with function attributes when the first instruction is a may throw. | |
308 | this does depend on the AC->assumptions ordering. but i don't think it depends on the map ordering since we only do lookup in the map. i changed the iteration order such that it is deterministic. even if AC->assumptions isn't. | |
316 | i added a comment below on where the position gets adjusted. | |
330 | i changed buildMapping such that i can filter assume that don't have true as argument. this way we don't affect them. | |
344 | here is the position adjustment in the second loop |
I wanted to get a feeling but run into this:
opt: /data/src/llvm-project/llvm/lib/Analysis/AssumeBundleQueries.cpp:154: llvm::RetainedKnowledge llvm::getKnowledgeForValue(const llvm::Value*, llvm::ArrayRef<llvm::Attribute::AttrKind>, llvm::AssumptionCache*, llvm:: function_ref<bool(llvm::RetainedKnowledge, llvm::Instruction*)>): Assertion `!!RKCheck && "invalid Assumption cache"' failed 2.»-Running pass 'Combine redundant instructions' on function '@clause_AllIndexedClauses'
I was running O3 on the LLVM-Test Suite SPASS clause.c file.
however the compile time cost is very high, originally around +15% after some tweaking i lowered it to 9%.
from what i saw the cause of the slowdown is cause by salvageKnowledge that generate lots of extra instructions and many part of the optimizer scale on the number of instruction.
here is the time taken by the parts i changed/added in the tweaked version:
0.43% in salvageKnowledge
0.01% in assumes Queries
0.70% in the assume simplify pass (i added it in 4 places in the pipeline as part of tweaks)
although they could probably be optimized, they are not a big part of the slowdown.i still have a few idea to try out and there is also lots of arbitrary decision i made during development that could be guided by measurement.
so the current overhead can surely be lowered but i don't think i can get it to a reasonable level without significantly limiting the preserved information.i didn't start measuring the runtime impact but i don't expect it to be good yet. since droppable uses are not widely in place.
any thought ?
First, we should profile where assumes are created, how many, and with what information (=attributes).
I'm usually not to concerned with the initial numbers as long as we haven't addressed the low hanging fruits.
We should also verify it is the number of instructions that is the problem. It could be other things, e.g., additional information or just handling of the operand bundles.
We should save some instructions moving alignment assumptions to operand bundles but that is something else.
llvm/include/llvm/Analysis/AssumeBundleQueries.h | ||
---|---|---|
126 | Documentation, please | |
163 | Nit: This extracts the or Extract the | |
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
230 | Can't we include analysis headers from here? transform should have a dependence on analysis anyway, right? If not, we can move it into the LLVMContext definition or some place like that. | |
291 | OK, before makes sense. We have a test, great. Thanks for checking! | |
llvm/utils/lit/lit/main.py | ||
276 ↗ | (On Diff #261890) | If this is because of NOEXE in the test suite, that is fixed right now elsewhere. Either way, this does not belong here ;) |
i will address comments soon but i made a patch with the fixes to run the test-suite. D79458
llvm/utils/lit/lit/main.py | ||
---|---|---|
276 ↗ | (On Diff #261890) | yeah this was for the NOEXE. |
Is there a reason we do not have the simplification pass in the old PM?
Can you add the pass in the pipeline (somewhere) after the ones that create assumes? Maybe use EnableKnowledgeRetention in the pass pipeline to only schedule this pass if it is enabled so there is really no impact otherwise.
Otherwise this looks already fine to me.
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
---|---|---|
424 | Please add a comment here to explain the order in which things a run. |
add support for legacy passmanager and add assume simplify to the pipeline just after EarlyCSE
LGTM. There is one comment below you should address.
llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp | ||
---|---|---|
424 | ^^^^^^^ |
this has been committed but i put the wrong commit message so it has not been automatically closed.
here is the commit rGda100de0a68f.
Documentation, please