feat: add separate DCE pass #1902
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1902 +/- ##
==========================================
- Coverage 86.63% 86.63% -0.01%
==========================================
Files 195 197 +2
Lines 35777 35897 +120
Branches 32590 32710 +120
==========================================
+ Hits 30995 31098 +103
- Misses 2987 3004 +17
Partials 1795 1795
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| pub type PreserveCallback = dyn Fn(&Hugr, Node) -> PreserveNode; | ||
|
|
||
| /// Signal that a node must be preserved even when its result is not used | ||
| pub enum PreserveNode { |
There was a problem hiding this comment.
The callback mechanism may be a sledgehammer to crack a nail, I'm not sure if there are alternatives.
- One might just say that every node that shouldn't be removed should just be added as an entry-point (i.e. including Calls, TailLoops, etc.)
- I'm not quite clear how this interacts with hierarchy, but that probably works ok
- However, the issue here is that we want the default to be conservative, so we should assume Call/CFG/TailLoop are impure unless we have evidence otherwise. This would mean the default would require adding every Call/CFG/TailLoop as an entrypoint, which sounds like far too much work to inflict on the client. Instead we'd have to have an
add_removableor something (and the client could list such nodes).
- As it stands this enum does have "everything I could think of", so it might be appropriate to make the enum smaller and add
[non_exhaustive]
There was a problem hiding this comment.
I agree we shouldn't demand the user specify every Call/CFG/TailLoop as an entry point; the interface does seem a bit awkward but I don't have a better suggestion.
| #[derive(Clone, Default)] | ||
| pub struct DeadCodeElimPass { | ||
| entry_points: Vec<Node>, | ||
| preserve_callback: Option<Arc<PreserveCallback>>, |
There was a problem hiding this comment.
There should be some documentation of what these are, especially preserve_callback.
| pub type PreserveCallback = dyn Fn(&Hugr, Node) -> PreserveNode; | ||
|
|
||
| /// Signal that a node must be preserved even when its result is not used | ||
| pub enum PreserveNode { |
There was a problem hiding this comment.
I agree we shouldn't demand the user specify every Call/CFG/TailLoop as an entry point; the interface does seem a bit awkward but I don't have a better suggestion.
| /// Mark some nodes as entry-points to the Hugr. | ||
| /// The root node is assumed to be an entry point; | ||
| /// for Module roots the client will want to mark some of the FuncDefn children | ||
| /// as entry-points too. |
There was a problem hiding this comment.
Is "entry point" hyphenated or not? (I think not.)
| .run_validated_pass(hugr, |h, _| self.run_no_validate(h)) | ||
| } | ||
|
|
||
| fn run_no_validate(&self, hugr: &mut impl HugrMut) -> Result<(), ValidatePassError> { |
There was a problem hiding this comment.
Does the return type need to include ValidatePassError?
| } | ||
|
|
||
| // "Diverge" aka "never-terminate" | ||
| // TODO would be more efficient to compute this bottom-up and cache (dynamic programming) |
| match h.get_optype(n) { | ||
| OpType::CFG(_) => { | ||
| // TODO if the CFG has no cycles (that are possible given predicates) | ||
| // then we could say it definitely terminates (i.e. return false) |
There was a problem hiding this comment.
I'm probably missing something, but I'm not sure why we don't want to remove nodes that we can't prove terminate. It's true that doing so would change runtime behaviour (as is the case for any optimization), but would it change program semantics?
Found a discussion of this question here: https://stackoverflow.com/questions/2178115/are-compilers-allowed-to-eliminate-infinite-loops
Still non-breaking, so we leave Constant Folding as assuming inputs apply to
main: the DCE pass allows explicitly specifying entry points, constant folding specifiesmainif appropriate. (I could add a flag to ConstantFoldPass=preserve all FuncDefns/FuncDecls?)The callback mechanism generalizes the previous
might_divergemechanism but is significantly more general. (Too general??)closes #1807