Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Comb] Let parent op decide if cross-block canonicalizations should be allowed #7529

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maerhart
Copy link
Member

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?

@maerhart maerhart added the Comb Involving the `comb` dialect label Aug 19, 2024
@fzi-hielscher
Copy link
Contributor

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 [CombFolds] Relax block optimization barrier by pulling constants #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Comb Involving the `comb` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants