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

SCF IndexSwitch to nested If-Else #7670

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jiahanxie353
Copy link
Contributor

This transform is meant for dialects that inherently does not have a switch control statement (like Calyx)

@jiahanxie353
Copy link
Contributor Author

#7669 supports nested if-else control

also referencing calyxir/calyx#1177 and calyxir/calyx#1423 if they are relevant

@rachitnigam
Copy link
Contributor

This needs test programs before we can merge it. Please add some tests and tag me when that's done.

Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping this again.

@jiahanxie353
Copy link
Contributor Author

Bumping this again.

Thanks for the reminder, I'll change and push again soon!

@jiahanxie353
Copy link
Contributor Author

jiahanxie353 commented Nov 6, 2024

I think I should move to directory lib/Transforms/SwitchToIf.cpp since it's transforming an scf operation to another, not lowering to another dialect?

Also tagging code owners for their opinions @mikeurbach @mortbopet , thanks!

@rachitnigam
Copy link
Contributor

Yeah, I think that sounds like a good idea!

@rachitnigam
Copy link
Contributor

Let's add tests before making @mortbopet or @mikeurbach review this PR...

@jiahanxie353
Copy link
Contributor Author

bumping this
thank you for reviewing folks

Copy link
Member

@cgyurgyik cgyurgyik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one tiny suggestion.

This transform is meant for dialects that inherently does not have a switch control statement (like Calyx)

Another suggestion: make this a bit more descriptive (and declarative) so readers can more easily capture what's going on, e.g.:

Transforms scf.switch to a series of scf.if statements. This is necessary for dialects that don't support switch statements, e.g., Calyx. An example:

%0 = scf.index_switch %cond -> i32
  case 0 { ... }
  case 1 { ... }
  ...

=>

%c0 = arith.cmpi eq %0, 0
%c1 = arith.cmpi eq %0, 1
%0 = scf.if %c0 {
 ...
} else {
  %1 = scf.if %c1 {
    ...
  } else {
    ...
  }
}

lib/Transforms/IndexSwitchToIf.cpp Outdated Show resolved Hide resolved
@jiahanxie353
Copy link
Contributor Author

Thanks, refined the description and code based on your review!

@jiahanxie353
Copy link
Contributor Author

hi @rachitnigam , does this patch look good to you now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants