Conversation
tlively
left a comment
There was a problem hiding this comment.
Can you move the function definitions into a .cpp file?
| // use cases are either to send a single function, or to send a set of functions | ||
| // that all have the same heap type (and so if they all do not use some | ||
| // parameter, it can be removed from them all). | ||
| inline bool removeParameter(const std::vector<Function*> funcs, |
There was a problem hiding this comment.
There are an awful lot of preconditions that have to be met to use this function correctly. I wonder if there is some more encapsulated abstraction that would work as well.
There was a problem hiding this comment.
Maybe the comment isn't clear enough - I think it's not that complex? Or maybe I don't understand your concern.
Another way to phrase the comment might be: This function assumes you have already checked for "logical" correctness. It then simply removes the parameter, and if you made a mistake then you would be breaking the wasm. The function does check for "hard constraints" and returns false if it hits them, such constraints are things that basically would make the wasm not validate if applied, or otherwise be obviously wrong.
There was a problem hiding this comment.
The preconditions:
- All calls and call_refs must be passed in.
- Removing the parameter should not change semantics.
- All functions must have the same signature.
I guess this isn't as bad as I initially thought, but this is more preconditions than I would expect from a general-purpose utility that's supposed to be useful in many potential situations. I guess it's more of a special-purpose utility that we only expect to ever call in a couple places.
Why can't we collapse the call sites into a single DAE optimization? It seems unfortunate to have such similar optimizations that share so much code but are still separate.
There was a problem hiding this comment.
Thanks, I see what you mean better now.
I think this is somewhat specialized and not fully general purpose, yes. But it is a way to avoid duplication between the existing DAE which optimizes single functions, and a future signature-pruning pass which does the same logical operation but on entire heap types at once. With this shared code split out, that new pass will be very simple.
We could in principle put all that code into a single file, DAE.cpp, and then these methods would go in there. Then it would be obvious it's not truly general purpose. I'd still prefer to have a new file SignaturePruning.cpp, however, just to keep that pass nice and separate and simple.
One option that might achieve both goals is to put this code in a separate file, but not in a "general" place like ir/function-utils.h? It could perhaps be in a file remove-parameters.cpp which would live alongside DAE.cpp and SignaturePruning.cpp?
There was a problem hiding this comment.
remove-parameters.cpp sounds good to me!
|
Good idea to split it out to a cpp file, done. These don't need to be inlined so there is no speed downside here. |
tlively
left a comment
There was a problem hiding this comment.
LGTM with the code living in a more specialized location.
|
I named it |
| for (auto* call : callRefs) { | ||
| call->operands.erase(call->operands.begin() + index); | ||
| } |
There was a problem hiding this comment.
This doesn't seem to update call_refs' target types. Is that OK?
There was a problem hiding this comment.
Ah, good point. Yes, this is intentional (the caller is expected to update types in all the places, its easier if a single place does it), but it should be documented. I'll open a PR.
In preparation for removing dead arguments from all functions sharing a heap
type (which seems useful for j2wasm output), first this PR refactors that code
so it is reusable. This moves the code out of the pass into FunctionUtils, and
also generalizes it slightly by
(no other changes to anything).