circt
circt copied to clipboard
[Comb] Let parent op decide if cross-block canonicalizations should be allowed
See #6523
The conservative restrictions of comb canonicalizers across blocks are hindering many optimizations at various places. E.g., the sv.initial mentioned in above issue, but also the HW/Comb/Seq/LLHD produced by the Slang/Moore frontend, and the arcilator lowering pipeline where we mix comb with SCF operations.
This PR attempts to fix this by adding a trait that operations can attach if they don't want to allow canonicalizations/folding to happen across blocks or regions as suggested by @mortbopet in above issue.
We should probably invert the trait, but then we need to add it to way more operations and I don't know how to attach it to operations defined upstream (probably need to make it an interface then (which I think is more expensive at runtime?)?)
Which are the ops that should attach the trait?
I hope you don't mind me leaving my two cents here early. After #7124 I kept trying to engineer a solution but in the end this felt to me like I'm continuing to dig a hole that we don't want to be in in the first place. I only vaguely remember the ODM where this was discussed and haven't re-watched the recording yet, but from my current perspective having this barrier at all just does not seem like a good idea:
- Trying to impose an optimization barrier without upstream support feels to me like fighting a losing battle (e.g., the constant hoisting that doomed #7124 )
- Even if we limit the scope of the barrier it is still a very conservative solution to what (iirc) was the original goal, i.e., prevent shifting of complexity from one block to another. An optimization that does not end up increasing complexity (however we define it) for any block is probably still desirable (e.g., constant folders).
- If we say a region does not preserve Comb semantics, is this true just because we say so or is there more principled reason behind it?
Regarding the last point, I think it is important to clearly distinguish between "we're not doing this optimization because it is bad for a set of use-cases" vs. "we're not doing this because it violates semantics". In the end my hope was that when we tackle #6931, giving us more fine-grained control over the Comb optimizations, this problem would solve itself. Not sure how justified that hope is.