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

[Lower SCF/Standard to Calyx] Add support for scf.if #4843

Open
xerpi opened this issue Mar 16, 2023 · 6 comments
Open

[Lower SCF/Standard to Calyx] Add support for scf.if #4843

xerpi opened this issue Mar 16, 2023 · 6 comments
Labels
Calyx The Calyx dialect enhancement New feature or request

Comments

@xerpi
Copy link
Contributor

xerpi commented Mar 16, 2023

I was wondering if it would be a good idea to add lowering support of scf::IfOp in createSCFToCalyxPass .
createSCFToCalyxPass claims to handle BranchOpInterface but when it encounters a scf::YieldOp it asserts if it's not scf::WhileOp.

I've also tried using createConvertSCFToCFPass to lower scf::IfOp into cf and it works well for scf::IfOp, but the problem is that it also converts scf::WhileOp into a cf::BranchOp with back-edges, so createSCFToCalyxPass fails with:

error: CFG backedge detected. Loops must be raised to 'scf.while' or 'scf.for' operations.

Therefore I think it's better to support scf::IfOp directly.

Here's simple_arith_if.mlir, a port of simple_arith.mlir to use scf::IfOp instead of cf::CondBranchOp , which can help test the lowering of scf::IfOp when implemented:

func.func @main(%arg0 : i32, %arg1 : i32) -> i32 {
  %0 = arith.cmpi slt, %arg0, %arg1 : i32
  %1 = scf.if %0 -> i32 {
    %3 = arith.addi %arg0, %arg1 : i32
    scf.yield %3 : i32
  } else {
    scf.yield %arg1 : i32
  }
  return %1 : i32
}

(Thanks to @rachitnigam for the updated code.)

EDIT: You need to run the command:

./build/bin/circt-opt --lower-scf-to-calyx test.mlir 
@mikeurbach mikeurbach added enhancement New feature or request Calyx The Calyx dialect labels Mar 16, 2023
@cgyurgyik
Copy link
Member

I'm less involved these days, but this makes sense to me. WDYT @mortbopet ?

@mortbopet
Copy link
Contributor

I think it would be a good idea, and i'd expect that the existing lowering for BranchOpInterface probably can be repurposed almost 1:1 (or factored out/generalized/...).

As you mention, there is no good way in MLIR to selectively ask conversion passes to only consider a subset of dialect operations (unless they explicitly implement this) which leads us into situations like this.

@rachitnigam rachitnigam changed the title [Lower SCF/Standard to Calyx] Add support for scf::IfOp [Lower SCF/Standard to Calyx] Add support for scf.if Aug 18, 2023
@rachitnigam
Copy link
Contributor

rachitnigam commented Aug 18, 2023

The original program is too simple and can therefore be canonicalized to arith.select (which is now supported by the Calyx flow). Here is a slight more complex program for testing an implementation of this feature:

func.func @main(%arg0 : i32, %arg1 : i32) -> i32 {
  %0 = arith.cmpi slt, %arg0, %arg1 : i32
  %1 = scf.if %0 -> i32 {
    %3 = arith.addi %arg0, %arg1 : i32
    scf.yield %3 : i32
  } else {
    scf.yield %arg1 : i32
  }
  return %1 : i32
}

@jiahanxie353
Copy link
Contributor

Hi folks, I'd like to give it a shot!
I've looked into the problem of the current pass that it seems to assume scf.yield will only occur in scf.while, whereas it can actually occur in scf.if as well. But I would appreciate some guidance and pointers on how to get started, or if there is any particular consideration I need to be aware of.
Thanks in advance!

@rachitnigam
Copy link
Contributor

@jiahanxie353 is a Cornell student and interested in working on this. We had a quick chat about possible next steps. Jiahan is going to look into the shape of the SCF to Calyx pass and come up with some concrete questions about the pattern rewriter infrastructure so that we can make progress.

Also, for reference, here is the command that you need to run to trigger the error with the above program:

./build/bin/circt-opt --lower-scf-to-calyx test.mlir 

@jiahanxie353
Copy link
Contributor

Hi folks, I've been diving deep into this, but hit a bit of a roadblock. I've created a draft PR to give you a snapshot of where I am, what I've been trying, and the challenges that have me scratching my head.
If you get a chance, could you take a look and shoot over any thoughts or pointers? It'd be super helpful! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Calyx The Calyx dialect enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants