Account for unaddressed funcs in GlobalEffects#8644
Conversation
stevenfontanella
changed the title
| an indirect call and we don't need to include its effects in indirect calls. | ||
| */ | ||
| std::unordered_set<Function*> getAddressedFuncs(Module& module) { | ||
| struct AddressedFuncsWalker : PostWalker<AddressedFuncsWalker> { |
There was a problem hiding this comment.
Might as well make this a WalkerPass so it can be run on all the functions in parallel. Then instead of walkModule below, you would use walkModuleCode. Alternatively you could use ModuleUtils::ParallelFunctionAnalysis + walkModuleCode.
There was a problem hiding this comment.
+1 on using ParallelFunctionAnalysis
There was a problem hiding this comment.
WalkerPass can also run in parallel:
Line 530 in 04adfce
stevenfontanella
force-pushed
the
indirect-effects-address
branch
from
cafaa9b to
dd80924
Compare
aheejin
left a comment
aheejin
left a comment
There was a problem hiding this comment.
LGTM % some nits + Thomas's comments
| an indirect call and we don't need to include its effects in indirect calls. | ||
| */ | ||
| std::unordered_set<Function*> getAddressedFuncs(Module& module) { | ||
| struct AddressedFuncsWalker : PostWalker<AddressedFuncsWalker> { |
There was a problem hiding this comment.
+1 on using ParallelFunctionAnalysis
| statements in our IR, but we check separately for funcs that appear in | ||
| `ref.func`). | ||
| - It's exported, because it may flow back to us as a reference. | ||
| - It's imported, which implies it is `elem declare`d. |
There was a problem hiding this comment.
Maybe it's only me but 'it is elem declared' sounds a little confusing (because it is not, even though I get what you mean). How about just saying imported functions can be passed a reference (as you did with the exported functions)?
There was a problem hiding this comment.
Rephrased the comment
- It's imported, which implies it can be addressed (see Should importing or exporting a function count as declaring it for ref.func? spec#2072).
|
Will continue this after #8625 is in. |
stevenfontanella
changed the title
stevenfontanella
changed the title
stevenfontanella
force-pushed
the
indirect-effects-address
branch
from
dd80924 to
0af5402
Compare
|
Have a few more updates to do, will update after I'm done. Also, apologies for the force-push. I've been trying out JJ which force-pushes by default. I tried merging via JJ but it still chose to force-push for some reason. Will look into disabling this locally. |
|
Should be good now, PTAL |
4 participants
Part of #8615. We currently union the effects of all functions of a given type when computing the effects for indirect calls. Make this more precise by excluding effects of functions that never have their address taken.
Continued from #8628 which was accidentally merged.