This is an archive of the discontinued LLVM Phabricator instance.

[AssumeBuilder] Add assume-simplify pass
ClosedPublic

Authored by Tyker on Apr 22 2020, 1:00 AM.

Details

Reviewers
jdoerfert
Summary

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.

Diff Detail

Event Timeline

Tyker created this revision.Apr 22 2020, 1:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2020, 1:00 AM
Tyker updated this revision to Diff 259206.Apr 22 2020, 1:50 AM

split out removal of non-determinism

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.

Tyker updated this revision to Diff 260091.Apr 25 2020, 6:25 AM
Tyker marked 9 inline comments as done.

addressed comments.
fixed some issues, improve tests.

Tyker marked 2 inline comments as done.Apr 25 2020, 6:30 AM
Tyker added inline comments.
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

Tyker updated this revision to Diff 261890.May 4 2020, 12:03 PM
Tyker marked 11 inline comments as done.

i tested to add this pass in the pipeline of the new PM and run over the test-suite.

Tyker added a comment.May 4 2020, 1:44 PM

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

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..

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 ;)

Tyker added a comment.May 5 2020, 4:57 PM

i will address comments soon but i made a patch with the fixes to run the test-suite. D79458

Tyker updated this revision to Diff 262325.May 6 2020, 2:53 AM
Tyker marked 4 inline comments as done.

addressed comments.

Tyker added inline comments.May 6 2020, 2:56 AM
llvm/utils/lit/lit/main.py
276 ↗(On Diff #261890)

yeah this was for the NOEXE.
happy to know it is fixed.

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.

Tyker updated this revision to Diff 262615.May 7 2020, 4:58 AM

add support for legacy passmanager and add assume simplify to the pipeline just after EarlyCSE

jdoerfert accepted this revision.May 7 2020, 9:45 AM

LGTM. There is one comment below you should address.

llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp
424

^^^^^^^

This revision is now accepted and ready to land.May 7 2020, 9:45 AM
Tyker closed this revision.EditedMay 11 2020, 3:43 AM

this has been committed but i put the wrong commit message so it has not been automatically closed.
here is the commit rGda100de0a68f.