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 |
| // - It appears in an `elem` segment (note that we already ignore `elem declare` | ||
| // statements in our IR, but we check separately for funcs that appear in | ||
| // `ref.func`). |
There was a problem hiding this comment.
This condition is subsumed by the the previous condition, so is redundant. Maybe that's what the note is saying, but I think we can just remove this entirely.
There was a problem hiding this comment.
Removed the point but included a small note on it still to make it clear how it maps to Wasm (we drop elem statements but in the source Wasm they should still be considered referenced).
| std::unordered_set<Function*> addressedFuncs; | ||
| AddressedFuncsWalker walker(addressedFuncs); | ||
| walker.walkModuleCode(&module); | ||
| walker.walkModule(&module); |
There was a problem hiding this comment.
This isn't going to traverse the module in parallel. You need to call .run() on the pass or explicitly use a PassRunner if you want it to run in parallel. However, having all the threads updating the single addressedFuncs set in parallel would not be safe. Safe options include collecting a separate set for each function in parallel (possibly using ParallelFunctionAnalysis) and then combining them, or using a std::unordered_map<Name, std::atomic<bool>> that is pre-filled with an entry for each function so that the threads do not race to grow the map.
There was a problem hiding this comment.
Done. Included it in the existing pass to avoid having to create a ParallelFunctionAnalysis with the same logic.
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.