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.
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
Grand Unified Flow Analysis (GUFA) #4598
Grand Unified Flow Analysis (GUFA) #4598
Changes from 250 commits
4f1a6b1
52535db
3a4a894
c3e3e30
8dfedaa
5457df6
9a2e636
d18b933
13adac9
adf9227
aecfd18
eee2e30
0444b41
bcaea49
2270174
9d01a3b
0e496a9
f620b76
b74f427
e0d0592
4627648
265c6f2
152541f
6faaa19
34380e0
7f07af0
732d881
7c73a1f
243a960
e304cdf
6e29761
d6ccf0d
9c64e28
1dc3d6d
55c473b
de91e5c
3aeb649
77ada29
71d7589
5618fee
55b5e44
2dcaccb
d57ecd3
0f5fee1
fa0736c
2a0ac9a
9d2b4c1
7af8861
8f9f1e4
32f7766
2607dcb
141e6e2
e2632b6
b8af191
0559dd9
ceb2dcd
e835f56
25bb0ca
9061659
8759f6c
64bb162
1e3cf3e
208c220
972211b
2734f78
6870508
5110432
f3b538e
d1a25ab
0402cab
bab9e92
26fca84
2810bcd
4a12da0
9135f6b
caccaaf
7b5ed43
e312c97
2c17fd3
7da19ab
4f338ad
8f47f02
12a5b23
e52d735
4a9c365
1854e72
7179d2e
7d61a73
225f36d
13464df
d11181f
ba3a913
0ab23d4
e66dfb0
51d0356
68b8dd2
5f213cc
8272223
218a82e
439061a
35de732
5e14116
efa0d21
cd74ec1
2eb813c
0a9631a
84e7541
ed46901
55760eb
f2cad07
4055e61
d2550b2
10fccbe
167f8ce
73b0809
e7aae12
e17b5bc
1d15978
cb4ed59
e69625d
bee4bb6
48b8d23
7c3296f
1e8d548
1a42ee2
eb3c7d1
0df4a40
036876f
206f139
bd1214e
8a770ff
0cc8e91
947b7da
19b52cb
3ad6228
36cc3c3
b0add64
e310cdb
52a0b30
5da22cd
0d5d412
032ed51
5df4495
66f3017
cdd0391
da56b25
dcfbfc2
e06e7bc
f4fd087
54c0531
14f4e6c
b1c51a1
1c41a12
5aa3e15
7cf666a
224f3df
45556c3
c47f529
da8d09c
b58cf1e
a3ac146
b128bd8
4a752dc
b7851c8
2de2cac
727f7f0
eb9c718
aa9643f
143a004
8598306
728caf0
14714f8
6015ecb
67a5967
137eac4
3f64ccf
67e9ea2
7f5c287
7da0a3f
1069efe
facff67
1c5aa5c
03f8896
678b9b1
2f20952
55ef34d
40671be
cc75d3d
6c5affd
8f4ff06
ec3de02
2c3fb78
8196125
815e40f
53a7d60
b733ada
3ddd0a8
e67e76d
a710d52
e3d371e
97a9ae3
372d2bd
ac23076
5bac7e2
ee1eb57
7d3288e
b73b5cb
c07d878
93e3404
c46c375
0ff332f
556e59f
9825e67
789293f
8832b52
4ec50b6
1973c77
91ac663
d2aea40
e1e9138
1725861
41ae2f9
295bc67
4790669
5400a37
3d1051f
3c65662
ce6a45d
0d53d84
e4987cb
adef90c
95b8ea5
3704951
0da2d8c
68fcea1
483087c
7988682
c00db9e
c6c0769
8f9e2a6
8955856
1eed7e0
d556760
14d83ef
4a1ca02
8925759
5e3d67c
5198ccc
cd8aa1e
4c3842b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why can't we remove its children too? This is within
canRemove
, which checksEffectAnalyzer
to see if it has any side effects. So that we are here meanscurr
can be removed safely, no? The same question for this function's use inreplaceWithUnreachable
.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.
Very good point! Yes, this could be a lot better. I fixed it to use
ShallowEffectAnalyzer
which is all we need since we'll keep the children if we actually need them. This optimizes more code + it's faster.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.
I think using
ShallowEffectAnalyzer
is better, but what I was asking about was, why do we even need to keep children ifEffectAnalyzer.hasUnremovableSideEffects()
returnsfalse
?Some expression as a whole, including children, may not have a side effect even when some of children has one. For example,
(br $label0)
has a side effects, but the whole(block ... )
doesn't. I guess we don't encounter this case in this pass because we don't remove named blocks anyway, but actually in this case this block may be removable, because thisblock
as a whole does not have any side effects.So, if we are to preserve children anyway, using
ShallowEffectAnalyer
is better for sure. But in the previous code, when we were usingEffectAnalyer
, if that doesn't have any side effects, can't we remove the expression, including children, altogether? Do we even need to preserve children in that case as well?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.
Yes, you're right that the earlier code was silly, since if
EffectAnalyzer.hasUnremovableSideEffects()
returnsfalse
then we don't need the children.The new code seems optimal: we check for shallow effects which ignores the children, which allows us to remove more things, and then we do keep the children (if we need them).
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.
Isn't it more optimal if we don't keep children, because we can remove more code that way? I'm not sure why we need to keep children when
EffectAnalyzer.hasUnremovableSideEffects
returns false, which means the expression as a whole, including all children, doesn't have any side effects.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.
You're right, we don't need to keep children in that case. Sorry if I wasn't clear before. Yes, that was an issue with the old code.
Does the new code make sense though? The key thing is that if the children are not actually needed then they'll get removed somehow - either right now, or later in Vacuum/DCE. So if we use
EffectAnalyzer.hasUnremovableSideEffects
and remove the children then we're just removing them eagerly here, but the final result is the same. But it's better to check for shallow effects here, since then we might find we can remove an instruction without removing its children - so the new code handles more cases, without losing anything (anything but the eager removal).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.
Is that always the case? There are cases when a child has a side effect but the parent, including all children, may not. Like the case I wrote above:
Also
try
-delegate
is a similar case; a singletry
-delegate
may have a side effect (becausedelegate
can target an outertry
) but the outertry
as a whole may not have any side effects.But yeah, these may not matter because we exclude
block
s andtry
s incanRemoveStructurally
or somewhere. But I wasn't sure it's always the case that "a child is not needed it will be removed by Vacuum eventually". Maybe it's true except the case ofblock
andtry
, which we handle separately.I think using
ShallowEffectAnalyzer
is better in terms of speed, because it's not O(n^2).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.
Oh, right! Sorry for not understanding you before. I forgot about the possibility of the combination having fewer effects.
Yes, I think you're right. In principle this is an issue. However,
canRemoveStructurally
rules those cases out as you said. I think any time that the combined expression has no effects, but the pieces of it do if they are separate, are cases that we can't remove other pieces.I added a comment here.
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.
OK now I think I understand better. Then what's the difference between the ones we exclude in
canRemoveStructurally
and the ones ingetDroppedUnconditionalChildrenAndAppend
?In
canRemoveStructurally
, we exclude namedblock
s,try
s, andpop
s. IngetDroppedUnconditionalChildrenAndAppend
, we excludeif
s andtry
s. But I think the thing they check should be the same, namely, they check whether the expression itself can be removed, regardless of its chlidren can be removed or not, which will be dropped separately.So my question is, 1. Are they really the same? 2. If so, do we need two places to check this? 3. And, doesn't
getDroppedUnconditionalChildrenAndAppend
need to excludepop
and namedblock
s as well, in case it is used in places other than GUFA? So what I mean is, can we check all these withingetDroppedUnconditionalChildrenAndAppend
and removecanRemoveStructurally
altogether? We are currently excluding expressions in three places (canRemove
,canRemoveStructurally
, andgetDroppedUnconditionalChildrenAndAppend
), which is confusing. If we can remove one of them that will be an improvement.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.
Hmm, I think this can actually be simplified a lot more, but it may be better to do that as a follow-up, given that this PR was hold up for too many weeks due to my laziness 😅 Sorry for that. I tried to do the follow-up in #4787.
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.
Why is this beneficial? Doesn't this add expressions and increase the code size? I understand this pass is supposed to increase the code size that can be cleaned up by other passes such as DCE and Vacuum, but that we are in this
else
clause means the dropped expression has some side effects that cannot be removed. Then can DCE or Vacuum remove it? For example, if the dropped part containscall
s orreturn
s, DCE/Vacuum wouldn't be able to remove it and we still need to run them.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.
There is some risk here, that is true. Perhaps this should only be done when not optimizing for size? However, propagating the constant to here will potentially open up opportunities to remove other code, so I'm not sure. Will add a comment now but let's keep thinking about it.
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.
Maybe we disable this part of code and do a quick check on perf and size, if that's easily doable?
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.
Testing now, code size is 1.6% smaller with this optimization. It's harder to test speed (we don't have great wasm GC benchmarks for this kind of thing), but emitting more constants is almost always better. So at least on this one example (j2wasm application) it looks worth doing.
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.
Hmm does that mean DCE or Vacuum was eventually able to remove those side-effect-having expressions? I was like, even if we emit an additional constant, if we cannot remove the original expression, that original expression still needs to be run..
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.
Yeah, there's no guarantee we can remove it. But sometimes dropping an expression lets us remove parts of it:
Now that the select is dropped we don't need it, and vacuum will remove it and just leave the calls (and maybe the condition).
(Not sure if that's the main factor here though. Could also be that we don't remove any dropped code, but the constant lets us remove other code...)
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.
I'm wondering what if move all meaningful reduction code from vacuum pass to some common util file and reuse routines from this file for vacuum and this pass? At least for those functions that don't require a global analysis of the program, but only subexpressions. This will allow you to avoid code overgrowth and its subsequent removal through a separate pass. It will also allow you to evaluate the benefit of constant propagation ahead of time (at least in local cases).
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.
@MaxGraey The vacuum code is already reusable right now, isn't it? We reuse it from here already just by doing
runner.add("vacuum");
. If you mean we just need a subset of vacuum, I'm not sure that's true, we need basically all of it.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.
runner.add("vacuum")
run on per function level right? But It can't provide a feedback. I mean something liketryVacuum(transform(expr))
and iftryVacuum
return false (which mean you can't cleanup code)just don't replace current expression to transformed due to code overgrowthThere 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.
@MaxGraey I see. I think that might make sense to do when optimizing for size perhaps. But the numbers from a few comments up show that even if we increase size slightly in a temporary way, the constants that we propagate end up helping more. So I think for now this is good enough, but maybe we can do better later.