Improve indirect call effects in GlobalEffects#8644
Improve indirect call effects in GlobalEffects#8644stevenfontanella wants to merge 3 commits intomainfrom
Conversation
stevenfontanella
changed the title
| ElementUtils::iterAllElementFunctionNames( | ||
| &module, [&addressedFuncs, &module](Name func) { | ||
| addressedFuncs.insert(module.getFunction(func)); | ||
| }); |
There was a problem hiding this comment.
No need to do this separately. Walking the module code will already walk all the element segments and find the RefFunc expressions inside them.
| 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
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
| ;; CHECK-NEXT: (nop) | ||
| ;; CHECK-NEXT: ) | ||
| (func $f (param $ref (ref $only-has-effects-in-not-addressable-function)) | ||
| ;; The type $has-effects-but-not-exported doesn't have an address because |
There was a problem hiding this comment.
| ;; The type $has-effects-but-not-exported doesn't have an address because | |
| ;; The func $has-effects-but-not-exported doesn't have an address because |
| ;; it's not exported and it's never the target of a ref.func. | ||
| ;; We should be able to determine that $ref can only point to $nop. | ||
| ;; TODO: Only aggregate effects from functions that are addressed. | ||
| ;; So the call_ref has no potential targets and thus no effects. |
There was a problem hiding this comment.
| ;; So the call_ref has no potential targets and thus no effects. | |
| ;; So the call_ref has no potential targets with effects. |
It has a target, $nop (without effects), so this was slightly confusing.
| } | ||
|
|
||
| // Type -> Type | ||
| // Do a DFS up the type heirarchy for all function implementations. |
There was a problem hiding this comment.
| // Do a DFS up the type hierarchy for all function implementations. |
Just a drive-by typo fix
| 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)?
3 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.