[SCFToCalyx] Fix improper usages of RewritePatterns

[richardxia]

This is the issue we encountered in #4353, but since we only landed a workaround to the root cause issue, I'm breaking this out into a separate issue.

cc @mortbopet @mikeurbach

I think #4353 probably has most of the details, but just to summarize, the SCFToCalyx convertor is misusing RewritePatterns in a few places, where it is not modifying the target op and instead inserting a new op, relying on a later RewritePattern to eventually delete the targeted op. This is problematic because the PatternRewriter infrastructure is designed to match on a certain pattern and then rewrite it into some other form, and there are assumptions it makes about idempotency and keeping track of mutations.

I don't know the SCFToCalyx code very well, but I think one way to fix this would be to take all of the addOncePattern() patterns defined in that code and rewriting them to not use the PatternRewriter system at all, instead keeping track of state on a custom class local to the SCFToCalyx converter.

We probably want to work on this sooner rather than later, since we don't know how long our workaround will continue working as upstream MLIR continues to change.

[teqdruid]

This (we strongly suspect) is causing tests to fail and blocking an llvm submodule bump. As discussed in the ODM today, we are XFAIL'ing the following tests:

Failed Tests (6):
  CIRCT :: Conversion/PipelineToCalyx/convert_pipeline.mlir
  CIRCT :: Conversion/SCFToCalyx/cider_source_location.mlir
  CIRCT :: Conversion/SCFToCalyx/convert_controlflow.mlir
  CIRCT :: Conversion/SCFToCalyx/convert_memory.mlir
  CIRCT :: Conversion/SCFToCalyx/convert_simple.mlir
  CIRCT :: Conversion/SCFToCalyx/errors.mlir

[mortbopet]

Thank you for raising the issue. I'll see if i can get to it if i have some free cycles, but won't promise anything. Was there ever a resolution on the ownership model of code in CIRCT? I'm happy to "own" Handshake code, but it's a bit of a stretch for me to also include Calyx inside that umbrella (even though I authored the initial implementation of SCFToCalyx). Ideally, I'd see the Calyx (Cornell) people take ownership of these parts of CIRCT (CC @rachitnigam, @matth2k, @andrewb1999).

[rachitnigam]

@andrewb1999 would you happen know what changes we can make to fix this?

[rachitnigam]

Also, apologies for not seeing this earlier! I was not getting notifications for Calyx labeled issues in the repo. @mortbopet, yes, figuring out an ownership policy would be useful. I think long term we should take over ownership of some of the Calyx conversion passes but might need some help while transitioning

[rachitnigam]

Also CC @cgyurgyik in case he has some spare cycles

[mikeurbach]

I think that defining the ownership of the Calyx parts and helping transition ownership to the Calyx experts would be great! I'm certainly happy to help in the transition, and as someone who has had my hand in the existing code, I'm happy to keep maintaining it. @mortbopet let me know if you get a chance to look at this, and I'll do the same if I free up a bit to take a look.

[rachitnigam]

Thanks @mikeurbach! That would be super useful

[mikeurbach]

I've got a little time to look into the immediate failure this evening, I'll post here if I make any headway.

[mikeurbach]

It looks like there are a couple things. For one, the iteration counting in MLIR was fixed (again), and now we actually need to set this to 1:

circt/lib/Conversion/SCFToCalyx/SCFToCalyx.cpp

Line 1264 in 5cb3856

config.maxIterations = 0;

With it set to 0, nothing happens, which is the root cause of why the tests started failing with the latest bump in #4570.

When I set it to 1, I get the ASAN issues described in #4378.

Since I'm in here, I wanted to take a look at actually fixing the ASAN issue while also re-enabling the tests. The root cause appears to be the CleanupFuncOps pattern, which erases the FuncOp in its partiallyLowerFuncToComp, which is wrapped by updateRootInPlace. updateRootInPlace re-enqueues the modified op to the worklist, even if we really only plan to run one iteration through the work list, the use-after-free occurs when adding the op, before the check to terminate iteration on the worklist.

I was tempted to just remove the call to updateRootInPlace, but we appear to do lots of mutation outside the rewriter in the various partiallyLower calls. ASAN didn't complain, but that's a ticking time bomb, so I'd rather not go that route.

I think the simplest change to fix the ASAN issue is to have CleanupFuncOps implement matchAndRewrite directly, rather than using the partiallyLower machinery. This allows it to cleanly erase the op without having updateRootInPlace get called and an erased op re-enqueued. We could potentially make partiallyLowerFuncToComp not a pure virtual method in case subclasses plan to just define matchAndRewrite themselves, but for now I just stubbed it out in CleanupFuncOps.

We still have our manual hack to make sure each pattern really only applies once, which I tried to remove by using the maxNumRewrites = 1 option to the driver, but when I removed that, this assertion fires:

circt/lib/Dialect/Calyx/Transforms/CalyxLoweringUtils.cpp

Line 285 in 5cb3856

"Memory already registered for memref");

It feels like we should be able to achieve the "apply me once" feature without the hack, but I will leave that for a future change, since ASAN is happy and the tests pass.

I noticed the Calyx tests were kind of brittle, and some argument re-ordering was breaking them, but otherwise, the tests pass and are ASAN clean. Here's a branch from the current LLVM bump branch: #4592.