Very simple revamp of dead code elimination. #28507
Conversation
mikebenfield
force-pushed
the
dead-code-class
branch
3 times, most recently
from
2867c59 to
460bc34
Compare
mikebenfield
force-pushed
the
dead-code-class
branch
from
460bc34 to
7201b3d
Compare
|
Alright see what you think of these changes. Also eliminates a couple new calls, in
|
mikebenfield
force-pushed
the
dead-code-class
branch
2 times, most recently
from
b1201cb to
e49b6f3
Compare
| Some(expression) => Some(self.reconstruct_expression(expression).0), | ||
| None => Some(self.reconstruct_expression(Expression::Identifier(member.identifier)).0), | ||
| }, | ||
| expression: member.expression.map(|expr| self.reconstruct_expression(expr).0), |
There was a problem hiding this comment.
In the None case, the identifier is the expression. Would the new code skip that case? Maybe it makes sense to reorganize the StructVariableInitializer so that identifier is optional?
There was a problem hiding this comment.
It seems to me there actually is no decent way to handle this to cover every circumstance. Indeed identifier may be the expression, but in such a case, depending on what the pass is doing, it may want to leave member as None, or it may want to make a new member.
compiler/passes/src/dead_code_elimination/dead_code_eliminator.rs
Outdated
Show resolved
Hide resolved
| /// The set of used variables in the current function body. | ||
| pub(crate) used_variables: IndexSet<Symbol>, | ||
| /// Whether or not the variables are necessary. | ||
| pub(crate) is_necessary: bool, |
There was a problem hiding this comment.
I'm for removing the pattern where we set is_necessary. It looks like instead we use the traversal to add used variables. Are we sure that there are not defaults in the traversal that might add something unwanted or skip something we'd want?
There was a problem hiding this comment.
I don't believe so - if we're visiting an expression, we do want to visit every subexpression of it, and we do explicitly control when we visit expressions via StatementReconstructor
There was a problem hiding this comment.
The test build options are defined here. Note that for most tests, they are disabled. If DCE is now enabled by default, it might be worth adding more comprehensive tests to ensure correctness. If it's looking like it would be a heavy lift, another option is to not enable DCE just yet, and only fold in the refactorings in this PR.
mikebenfield
force-pushed
the
dead-code-class
branch
2 times, most recently
from
bab0381 to
fcb14ea
Compare
|
Alright so here's what we discussed - don't eliminate any operations that may halt. |
mikebenfield
force-pushed
the
dead-code-class
branch
from
a5606da to
7d48645
Compare
|
We actually probably want to wait a bit before merging this. There is a discussion here about which hash and commit functions may halt, and we likely want to know the outcome of that discussion so we know specifically which functions we consider side effect free. |
Enable DCE by default. Eliminates useless associated function calls. Also threw in just a few changes to Display implementations for some nodes. Fixes #28504
mikebenfield
force-pushed
the
dead-code-class
branch
from
731e054 to
6912c23
Compare
|
Alright, this should be ready to go now - it's confirmed that all hash functions except the Pedersen ones work for all inputs. I've rebased and now the base branch is |
Enable DCE by default. Eliminates useless
associated function calls.
Also threw in just a few changes to Display
implementations for some nodes.
Fixes #28504