-
Notifications
You must be signed in to change notification settings - Fork 871
[NFC] ConstraintAnalysis: Track relevant locals #8921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
kripken
wants to merge
24
commits into
WebAssembly:main
Choose a base branch
from
kripken:constraint.relevant
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+367
−7
Open
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
ab1c83b
fix
kripken a72513d
Merge remote-tracking branch 'origin/main' into constraint.deredundant
kripken 642229c
fix sorting
kripken c839fe5
go
kripken 45c4788
fix
kripken 06c03af
form
kripken 7266144
form
kripken 72eba6a
fix
kripken 57b156b
fix
kripken 2c48869
test
kripken f3abddf
fix
kripken baace23
assert
kripken 39d583d
Merge remote-tracking branch 'origin/main' into constraint.relevant
kripken c33dc89
fix
kripken 637663f
Merge remote-tracking branch 'origin/main' into constraint.relevant
kripken 83f1424
simpl
kripken 745ecdb
simpl
kripken ce848d0
try
kripken 2aee764
fix
kripken 8f47c16
go
kripken 06ae43c
rename as suggested
kripken 7502a4c
fix
kripken 3026d40
format
kripken 8bb6a22
simpl
kripken File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,32 +84,90 @@ struct ConstraintAnalysis | |
| // state in the function. | ||
| bool ignoreBranchesOutsideOfFunc = true; | ||
|
|
||
| // A relevant local is one that is used as part of an expression that we can | ||
| // optimize (often, many locals are irrelevant). | ||
| std::vector<bool> relevantLocals; | ||
| // Track local copies too, as if one local is relevant, it can make another | ||
| // relevant. We store pairs here of key=target, value=sources, which is the | ||
| // direction we will flow in the analysis: if we check x == 10, making it | ||
| // relevant, and x = y earlier, then we must track that source, y, so that we | ||
| // know what it writes to x. | ||
| std::unordered_map<Index, std::vector<Index>> localCopySources; | ||
|
|
||
| void maybeMarkRelevant(Expression* curr) { | ||
| // If this parses into a constraint on a local, that local is relevant. | ||
| if (auto parsed = LocalConstraint::parseCondition(curr)) { | ||
| relevantLocals[parsed->local] = true; | ||
| if (auto* other = std::get_if<Index>(&parsed->constraint.term)) { | ||
| relevantLocals[*other] = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| void doWalkFunction(Function* func) { | ||
| relevantLocals.assign(func->getNumLocals(), false); | ||
|
|
||
| Super::doWalkFunction(func); | ||
| } | ||
|
|
||
| // Store the actions we care about. | ||
| void addAction() { | ||
| if (currBasicBlock) { | ||
| currBasicBlock->contents.actions.push_back(getCurrentPointer()); | ||
| } | ||
| } | ||
|
|
||
| void visitLocalSet(LocalSet* curr) { addAction(); } | ||
| void visitUnary(Unary* curr) { addAction(); } | ||
| void visitBinary(Binary* curr) { addAction(); } | ||
| void visitRefEq(RefEq* curr) { addAction(); } | ||
| void visitRefIsNull(RefIsNull* curr) { addAction(); } | ||
| void visitLocalSet(LocalSet* curr) { | ||
| addAction(); | ||
| if (auto* get = curr->value->dynCast<LocalGet>()) { | ||
| // TODO: handle tees once we handle them elsewhere | ||
| localCopySources[curr->index].push_back(get->index); | ||
| } | ||
| } | ||
|
|
||
| void visitUnary(Unary* curr) { | ||
| addAction(); | ||
| maybeMarkRelevant(curr); | ||
| } | ||
|
|
||
| void visitBinary(Binary* curr) { | ||
| addAction(); | ||
| maybeMarkRelevant(curr); | ||
| } | ||
|
|
||
| void visitRefEq(RefEq* curr) { | ||
| addAction(); | ||
| maybeMarkRelevant(curr); | ||
| } | ||
|
|
||
| void visitRefIsNull(RefIsNull* curr) { | ||
| addAction(); | ||
| maybeMarkRelevant(curr); | ||
| } | ||
|
|
||
| static void doStartIfTrue(ConstraintAnalysis* self, Expression** currp) { | ||
| // We are right after the condition, so we are in the block before the If's | ||
| // branching. Mark the If as the brancher (unless in unreachable code). | ||
| if (self->currBasicBlock) { | ||
| self->currBasicBlock->contents.brancher = *currp; | ||
| } | ||
| if (auto* iff = (*currp)->dynCast<If>()) { | ||
| self->maybeMarkRelevant(iff->condition); | ||
| } | ||
| Super::doStartIfTrue(self, currp); | ||
| } | ||
|
|
||
| static void doEndBranch(ConstraintAnalysis* self, Expression** currp) { | ||
| if (self->currBasicBlock) { | ||
| self->currBasicBlock->contents.brancher = *currp; | ||
| } | ||
| if (auto* br = (*currp)->dynCast<Break>()) { | ||
| if (br->condition) { | ||
| self->maybeMarkRelevant(br->condition); | ||
| } | ||
| } else if (auto* brOn = (*currp)->dynCast<BrOn>()) { | ||
| self->maybeMarkRelevant(brOn->ref); | ||
| } | ||
| Super::doEndBranch(self, currp); | ||
| } | ||
|
|
||
|
|
@@ -118,11 +176,41 @@ struct ConstraintAnalysis | |
| // Body is unreachable, no entry block. | ||
| return; | ||
| } | ||
| // TODO: optimize for speed, find relevant locals etc. | ||
|
|
||
| computeRelevantLocals(); | ||
| flow(); | ||
| optimize(); | ||
| } | ||
|
|
||
| // Every relevant local makes the things it is copied to relevant as well. | ||
| void computeRelevantLocals() { | ||
| // We'll start from all relevant locals, and flow from there. | ||
| UniqueDeferredQueue<Index> work; | ||
| for (Index i = 0; i < relevantLocals.size(); i++) { | ||
| if (relevantLocals[i]) { | ||
| work.push(i); | ||
| } | ||
| } | ||
| if (work.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| // Flow. | ||
| while (!work.empty()) { | ||
| auto curr = work.pop(); | ||
| assert(relevantLocals[curr]); | ||
| if (auto iter = localCopySources.find(curr); | ||
| iter != localCopySources.end()) { | ||
| for (auto source : iter->second) { | ||
| if (!relevantLocals[source]) { | ||
| relevantLocals[source] = true; | ||
| work.push(source); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Flow infos around until we have inferred all we can about the constraints | ||
| // in each location. | ||
| void flow() { | ||
|
|
@@ -132,6 +220,10 @@ struct ConstraintAnalysis | |
| auto& entryConstraints = entry->contents.startConstraints; | ||
| auto* func = getFunction(); | ||
| for (Index i = func->getVarIndexBase(); i < func->getNumLocals(); i++) { | ||
| if (!relevantLocals[i]) { | ||
| // No point to apply a constraint to an irrelevant local. | ||
| continue; | ||
| } | ||
| auto type = func->getLocalType(i); | ||
| // TODO: support tuples | ||
| if (type.size() == 1 && LiteralUtils::canMakeZero(type)) { | ||
|
|
@@ -165,7 +257,8 @@ struct ConstraintAnalysis | |
|
|
||
| // Find the constraints sent to this specific successor, if there is a | ||
| // branch, and use them. | ||
| if (auto branch = getBranchConstraints(block, out)) { | ||
| if (auto branch = getBranchConstraints(block, out); | ||
| branch && checkRelevancy(*branch)) { | ||
| auto sentConstraints = constraints; | ||
| sentConstraints.approximateAnd(branch->local, branch->constraint); | ||
| // If anything changed at the start of the target block, flow onwards. | ||
|
|
@@ -223,6 +316,9 @@ struct ConstraintAnalysis | |
| if (!parsed) { | ||
| return; | ||
| } | ||
| if (!checkRelevancy(*parsed)) { | ||
| return; | ||
| } | ||
|
|
||
| auto localConstraints = constraints.get(parsed->local); | ||
| Result result = localConstraints.proves(parsed->constraint); | ||
|
|
@@ -320,6 +416,10 @@ struct ConstraintAnalysis | |
| void applyToConstraints(Expression* curr, | ||
| BasicBlockConstraintMap& constraints) { | ||
| if (auto* set = curr->dynCast<LocalSet>()) { | ||
| if (!relevantLocals[set->index]) { | ||
| // No point to apply a constraint to an irrelevant local. | ||
| return; | ||
| } | ||
| if (Properties::isSingleConstantExpression(set->value)) { | ||
| // Apply a constraint to this value. | ||
| auto value = Properties::getLiteral(set->value); | ||
|
|
@@ -333,6 +433,33 @@ struct ConstraintAnalysis | |
| } | ||
| } | ||
| } | ||
|
|
||
| // When we are about to use or apply a constraint to a local, it must be on a | ||
| // relevant one - otherwise we misidentified which are relevant, which could | ||
| // lead to missed opportunities or misoptimizations. This returns true if we | ||
| // are operating on proper, relevant data. Normally this is all that can | ||
| // happen, but intermediate optimizations can make things become relevant, | ||
| // consider this: | ||
| // | ||
| // x == (y < 10) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this change not NFC because of this? IIUC, we're losing a very small amount of optimization power here. |
||
| // | ||
| // The outer == is initially not relevant: we are comparing x to something we | ||
| // can't parse into a constraint's term. However, if we get lucky and optimize | ||
| // y < 10 into a constant, then it does become parseable, but because we did | ||
| // not consider x as relevant (and so we do not have all the relevant | ||
| // information about it), we must return false here and not operate on it | ||
| // (later optimization cycles can get to it). | ||
| bool checkRelevancy(const LocalConstraint& parsed) { | ||
| if (!relevantLocals[parsed.local]) { | ||
| return false; | ||
| } | ||
| if (auto* other = std::get_if<Index>(&parsed.constraint.term)) { | ||
| if (!relevantLocals[*other]) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
| }; | ||
|
|
||
| } // anonymous namespace | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this early return, since if
workis empty, the loop below will simply not run.