Constructing backedges

MLIR
#1

TL;DR: Is there a method for constructing backedges in MLIR? If not, we should add one.

With the addition of graph regions, backedge construction has become an issue. In CIRCT, we have a support utility named BackedgeBuilder which constructs a dummy op with the result type of a specified type. We then use the dummy op result to construct the op which consumes the fed back value with the dummy value, then replace the dummy value when the actual op gets constructed.

Up until a few days ago, we used an unregistered operation as the dummy op and set the flag in MLIRContext to allow unregistered ops, which wasn’t great. We’ve moved to a registered dummy op in our hw dialect, but that introduces a dependency on the hw dialect from the support library (which shouldn’t have any dialect deps). BackedgeBuilder is templated (to specify the dummy op), but other code in the support library uses it, so that code would have to become templated as well, which may mean other code would have to (rinse and repeat). This feels like a hacky solution just to avoid a dependency.

Is there a better way?

#2

Using a dummy operation is what the MLIR parser used to do, but after we started hardening unregistered dialects that got a bit awkward/broken. What it does now is create an instance of builtin.unrealized_conversion_cast, which is always registered (hence the builtin.). Realistically you could do the same thing in circt, which would remove the need for a special “dummy”/unnecessary dependencies/etc.

– River

#3

Thanks River! That works: [Support] Backedges: Use always registered `builtin.unrealized_conversion_cast` by teqdruid · Pull Request #2948 · llvm/circt · GitHub

Should I be pushing this upstream? There’s really nothing CIRCT-specific about it. We’ve also got a ValueMapper which accounts for backedges.

#4

I’m not Mehdi, but I’ll take that as a compliment :wink:

Do you have links to how that class gets used in Circt?

1 Like
#5

Shit, sorry. I really shouldn’t be allowed in front of a keyboard until my coffee kicks in!

Sure.

Ex 1. Init circt/ESIPasses.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub, get backedge circt/ESIPasses.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub, replace circt/ESIPasses.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub.

Ex 2. init circt/StandardToHandshake.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub, get edge circt/StandardToHandshake.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub, replace circt/StandardToHandshake.cpp at 76d640c37bd469a792d0dba40fee9603ab1cfb4e · llvm/circt · GitHub

BackedgeBuilder checks that all the backedges get replaced via RAII.

I’m not necessarily attached to the existing API if you think a different one makes sense.

#6

If this has demonstrably improved interacting with graph regions, I’d say we should upstream it. We can discuss specifics of the API in code review.

– River