[SCF-to-Calyx] Fix Ordering Bug

[calebmkim]

Fixes #5521.

The main thing was adding the WhileScheduleable to the BlockScheduler during the BuildGroupOps pattern instead of the BuildWhileGroup pattern, which means that all the block schedulables are added during the same pattern, preventing the order mismatching that was going on before. To do this, I needed to add initGroups (i.e., the groups that execute before the while loop that initialize the loop variables) to the LoopLoweringStateInterface.

I also added a test case in convert_controlflow.mlir.

[andrewb1999]

Great job tracking this down! One question I have is whether the BuildWhileGroups pattern is even necessary anymore. Could you just move all of that code into the BuildOpGroups function and remove the pattern entirely? If we need a BuildOpGroups function for WhileOps anyways I don't see a reason why BuildWhileGroups would be necessary anymore.

[calebmkim]

hmm that's a good point. I can try to do that and see if there's any problems that I'm not thinking of. But it does seem like we might be able to get rid of BuildWhileGroups

Edit: ok, so after trying this, it's erroring out. There seems to be some dependency issue with the yieldOp that is giving us an error if we try to handle WhileOps in the same pattern as yieldOp. I'm not sure exactly what's going on, but I'm thinking we should keep this PR small for now ( so the bug can get fixed ASAP)

[mikeurbach]

Is a similar change needed in LoopScheduleToCalyx? We have tried to share code between these passes, but at the moment, some functionality is duplicated.

Besides that, I left some nitpicky comments, and this looks good to me otherwise. @mortbopet do you have any other thoughts? This seems like a reasonable way to address this.

[mortbopet]

My only thought is that i'd like to see the whole scheduleable-implementation to be modified to be an association between operation : {startScheduleables, innerScheduleable (some recursive structure), endScheduleables}. Why? I think it's safe to say that the IR ingested by this pass models sequential "software-like" code. By extension, it should then be reasonable to expect than the ordering of blocks that need to be scheduled follows the order of which the operations that originated those blocks appear in the input IR.

That is, when we encounter e.g. an scf.while operation, we know it's general position within the total set of ordered blocks relative to all other operations at the input level, we know which init groups it has (those you just implemented). we know the inner groups (recursing into the body of the loop) and we know the exit groups (the loop latch).

It could be cool to see the above, but if you just want to get something working, then i assume this can suffice 👍 (although if you're interested in developing a cool, scalable, reuseable way of doing this stuff, then... 👀).

[calebmkim]

Is a similar change needed in LoopScheduleToCalyx? We have tried to share code between these passes, but at the moment, some functionality is duplicated.

Yeah, I'm realizing now that you can create a similar bug in the LoopScheduleToCalyx pass. But I know @andrewb1999 is working on some modifications to this pass as well. Maybe I can talk discuss w/ him to discuss if his implementation will still have this problem, and if so, maybe I can try to help w/ that.

It could be cool to see the above, but if you just want to get something working, then i assume this can suffice 👍 (although if you're interested in developing a cool, scalable, reuseable way of doing this stuff, then... 👀).

Yeah, I definitely see how what I've done is more of a "band-aid" than the more "complete" solution that you mention. Currently I have a decent amount on my plate as it is, so I'd lean towards my quick solution. Although I would be interested in doing something like this (and improving the Calyx CIRCT Dialect in general) in the future.

[andrewb1999]

Yeah I'm working on a pretty significant rewrite of LoopScheduleToCalyx and this is one of the issues I'm trying solve in a better way.

My only thought is that i'd like to see the whole scheduleable-implementation to be modified to be an association between operation : {startScheduleables, innerScheduleable (some recursive structure), endScheduleables}. Why? I think it's safe to say that the IR ingested by this pass models sequential "software-like" code. By extension, it should then be reasonable to expect than the ordering of blocks that need to be scheduled follows the order of which the operations that originated those blocks appear in the input IR.

@mortbopet Could you expand a little bit on what you envision this looking like? I might be interested in implementing something like this as part of my LoopScheduleToCalyx rewrite (which could then end up being used as part of SCFToCalyx too).

Overall, I think this bandaid fix is good to merge soon and then we can work towards a better solution later.

[mikeurbach]

My two cents is to merge this, at least for SCFToCalyx, and probably for LoopScheduleToCalyx as well, depending on how much it will complicate what Andrew has in the works. This change fixes a real bug, and I don't think merging this makes it much harder to re-implement these passes the way we really want them to work.

[andrewb1999]

My two cents is to merge this, at least for SCFToCalyx, and probably for LoopScheduleToCalyx as well, depending on how much it will complicate what Andrew has in the works. This change fixes a real bug, and I don't think merging this makes it much harder to re-implement these passes the way we really want them to work.

That sounds good to me, a similar fix for LoopScheduleToCalyx shouldn't really affect my future changes much.

[mortbopet]

Bug fixes are good and sounds like we have more work around here already in the works... approved!

[mortbopet]

@andrewb1999

Pseudocode-y:
The first assumption is that whatever we're feeding into SCFToCalyx (or similar passes) models a sequentially executing IR (i.e. not a graph). Let's call this source IR S.

That means that the order of blocks that need to be scheduled when converting said graph must follow the order if which operations appears in S.

So whereas we right now have a single vector of "block scheduleables", that could instead be a mapping from operations to a struct that describes which blocks said operation wants to be scheduled, and in what ("intra-op") order.

using BlockSchedulingScope = DenseMap<Operation, List>

During conversion of operations, operations will insert their scheduleables into the blockschedulingscope map instead of just pushing them back onto a list.
And then finally, once we're done with converting all of the operations, we can materialize the schedule by

1. iterating over the original operations in S (ensuring source IR ordering) - maintaining inter-op order
2. emit the block scheduleables that each operation registered - maintaining intra-op order

I then imagine that BlockSchedulingScope doesn't have to be just a mapping from Operations to a list of scheduleables. If needed/useful, it could also be some recursive structure to suit that fact that some operations themselves define a scope (i.e. for/while loops).

[rachitnigam]

Merging this! @mortbopet can you give @calebmkim write access to the repo? He is one of the main Calyx developers these days

[mikeurbach]

As CIRCT is part of the LLVM github org, obtaining write access is the same as obtaining write access for LLVM: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. Basically, you get added to the LLVM org, and then you can merge to CIRCT.