Remove unused results in DAE2#8903
Conversation
Extend DeadArgumentElimination2.cpp to track and eliminate unused function return values in addition to dead parameters. Generalize the analysis graph by introducing a unified `Location` variant that handles both parameter and result locations across functions and function type trees (`FuncResultLoc` and `TypeResultLoc`). Extend value-flow tracking to follow return values flowing into function returns, including tracking tail calls (`tailCallees` and `tailCalleeTypes`) so that result usage constraints propagate correctly from callees to tail-callers. Update the fixed-point solver to propagate result usage bidirectionally across function implementations and type trees. When a function or type result is determined to be unused, update its signature result to `Type::none` during type rewriting and strip the return expressions and call site result types during optimization.
| // Function index and parameter index. | ||
| using FuncParamLoc = std::pair<Index, Index>; | ||
|
|
||
| // Function index identifying the function' result (we treat result tuples as a |
There was a problem hiding this comment.
| // Function index identifying the function' result (we treat result tuples as a | |
| // Function index identifying the function's result (we treat result tuples as a |
Also, how does an Index identify a result? Is this just the arity? Or is this mapping types to an Index in some manner?
There was a problem hiding this comment.
Reading above, I see parameters are also mapped to an Index, but I don't remember how that works. Might be worth a general comment here?
There was a problem hiding this comment.
The index is the index of the function whose result is being identified. That's the same as the first Index in FuncParamLoc, but we don't need a second index because there is at most one result.
There was a problem hiding this comment.
Wait, the comment already says "function index" in both locations 😆
There was a problem hiding this comment.
I did miss "function index" here, but the comment above says just "parameter index". Or, more fully, it says "Function index and parameter index". What is the "parameter index"? (it can't be equal to the function index, can it..?)
And, if we use a function index to identify a function's result, why not just call it a function index? Something in the design here seems confusing to me - I'm probably not remembering something basic, but I think that might be a good reason to explain the design more in a comment?
There was a problem hiding this comment.
Its purpose is to identify the location of a single function result in the scope of the entire module.
How else would you do this? We could use the function name, but we use function indices instead of names for efficiency.
There was a problem hiding this comment.
Its purpose is to identify the location of a single function result in the scope of the entire module.
What is a "single function result"? The entire result, or an index into the result, like with params, above?
In more detail: I know how to read using FuncParamLoc = std::pair<Index, Index>; - that is a function index and a param index. That makes total sense to me. But we then have using FuncResultLoc = Index; which confuses me. I would expect instead using FuncResultLoc = std::pair<Index, Index>;, entirely parallel to the Param case, which would indicate a function index and a result index?
There was a problem hiding this comment.
But in that framing the only possible result index is 0, so we can just drop it entirely. As the comment says, we treat tuples as units so there is never more than one result.
There was a problem hiding this comment.
Ok, thanks, that's the asymmetry then between params and results...
Why do we treat results as a tuple but not params, btw?
There was a problem hiding this comment.
That's how they're modeled in the IR, so it's simplest to model them that way in the analysis as well. Tracking individual elements of the result tuples would require tracking individual elements of tuples everywhere else in the analysis, too.
|
Ran 100k fuzzer iterations overnight with 30 copies of DAE2 in the fuzz_opt.py list of passes. |
| return join(*l, other); | ||
| } | ||
| if (std::get_if<FuncResultLoc>(&loc)) { | ||
| return join(std::get<FuncResultLoc>(loc), other); |
There was a problem hiding this comment.
Why do we do get_if and then get, rather than use the result of get_if as in the if above us?
There was a problem hiding this comment.
This looks like an oversight. I'll fix it.
| ) | ||
| ) | ||
|
|
||
| ;; Test 8.2: JS-Called via configureAll |
There was a problem hiding this comment.
Oops, missed this while reviewing and cleaning up Gemini output. Will remove.
| (func $callee (param $unused i32) (result i64) | ||
| (i64.const 0) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Tests that only add results are removed here because we now have more complete coverage of those situations in other files.
There was a problem hiding this comment.
Sounds good, though with the amount of tests here it is hard to keep it all in my head, and to make sure we didn't lose coverage.
| ) | ||
| (else | ||
| (local.get $unused) | ||
| (global.set $g |
There was a problem hiding this comment.
Before this change to the test, the result of the function was unused, so the i32.add would be removed entirely after this change to DAE2. Adding a global.set in place of returning the i32.add keeps it used and better preserves the original intent of the test.
| // For each parameter of this function, the list of parameters in direct | ||
| // callers that will become used if the parameter in this function turns out | ||
| // to be used. Computed by reversing the directForwardedParams graph. | ||
| std::vector<std::vector<FuncParamLoc>> callerParams; |
There was a problem hiding this comment.
It looks like callerParams was removed, but code behind #if DAE_STATS still uses it and will not compile.
There was a problem hiding this comment.
I'm currently working on a follow-up that updates all the DAE_STATS code.
Extend DeadArgumentElimination2.cpp to track and eliminate unused function return values in addition to dead parameters.
Generalize the analysis graph by introducing a unified
Locationvariant that handles both parameter and result locations across functions and function type trees (FuncResultLocandTypeResultLoc). Extend value-flow tracking to follow return values flowing into function returns, including tracking tail calls (tailCalleesandtailCalleeTypes) so that result usage constraints propagate correctly from callees to tail-callers.Update the fixed-point solver to propagate result usage bidirectionally across function implementations and type trees. When a function or type result is determined to be unused, update its signature result to
Type::noneduring type rewriting and strip the return expressions and call site result types during optimization.