[WIP] Conversion infrastructure

[greenhat]

I'm doing dialect conversion in OmniZK, so I'm drafting a pass infrastructure in pliron that resembles MLIR's. I'm using mostly the same names for types and functions.
It's still WIP and is not ready for review yet. I want to get your feedback. Let me know what you think of it.

TODO:

• extract 'thiserror' and 'anyhow' crates introductions as a separate PR;

[vaivaswatha]

I'm doing dialect conversion in OmniZK, so I'm drafting a pass infrastructure in pliron that resembles MLIR's. I'm using mostly the same names for types and functions. It's still WIP and is not ready for review yet. I want to get your feedback. Let me know what you think of it.

Thank you !. Please give me a couple days to get back to you on this.

[vaivaswatha]

I'm doing dialect conversion in OmniZK, so I'm drafting a pass infrastructure in pliron that resembles MLIR's. I'm using mostly the same names for types and functions. It's still WIP and is not ready for review yet. I want to get your feedback. Let me know what you think of it.

Thank you !. Please give me a couple days to get back to you on this.

I'm going through what / how MLIR does it in more detail. Also a friend pointed me to https://mlir.llvm.org/docs/Dialects/PDLOps/, something that MLIR is doing more recently.

[greenhat]

I'm doing dialect conversion in OmniZK, so I'm drafting a pass infrastructure in pliron that resembles MLIR's. I'm using mostly the same names for types and functions. It's still WIP and is not ready for review yet. I want to get your feedback. Let me know what you think of it.

Thank you !. Please give me a couple days to get back to you on this.

I'm going through what / how MLIR does it in more detail.

Sure. Initially, I tried to keep API closer to MLIR but some features (typed ops, etc) I was unable to implement in Rust, so I dropped them for now.

Also a friend pointed me to https://mlir.llvm.org/docs/Dialects/PDLOps/, something that MLIR is doing more recently.

This is so cool! I'm going to dig into it. Thanks!

[vaivaswatha]

I'm still contemplating the best way to do dialect conversions and other transforms on the IR. But meanwhile, I had some thoughts on the other changes in your PR: what do you think?

• Addition of thiserror and anyhow is cool, but can that be done as a separate PR, applying for the whole project? maybe even before the actual conversions PR.
• Seeing that a pass manager is orthogonal to your change (and as far as I can see, is only used in testing), can we skip it altogether and consider it a separate problem to solve at another time?

[greenhat]

I'm still contemplating the best way to do dialect conversions and other transforms on the IR. But meanwhile, I had some thoughts on the other changes in your PR: what do you think?

• Addition of thiserror and anyhow is cool, but can that be done as a separate PR, applying for the whole project? maybe even before the actual conversions PR.

Sure. What is your vision for error handling? I see that errors are used only in Verify trait with unwraps and asserts everywhere else. Do you plan to rely on force unwraps and asserts? On the one hand, I like it cause it is simple and gives you the exact location via stacktrace. But on the other hand, pliron might be used in an environment where crashing is not allowed. I suggest using hierarchical error types with thiserror #[from attribute to set the source. Optionally, we can even include a stacktrace captured via std::backtrace at the error origin or somewhere close to it. Ideally, we can even forbid unwraps on the crate level and use explicit #[allow... where unwraps are necessary.

• Seeing that a pass manager is orthogonal to your change (and as far as I can see, is only used in testing), can we skip it altogether and consider it a separate problem to solve at another time?

Yes, it is a thin wrapper for the passes list and not that useful at this point. I'll remove it for now.

[vaivaswatha]

What is your vision for error handling? I see that errors are used only in Verify trait with unwraps and asserts everywhere else.

The way I see it, unwraps and asserts, when they fail means that it's a bug in the core infrastructure. It shouldn't happen at all (ideally).

Recoverable errors are for when it isn't a bug in pliron. For example, something's wrong with the input program that it fails verification, or there's a syntax error in parsing etc. Conversion errors could be in that category too, i.e., not a compiler bug.

When I suggested that we do thiserror and anyhow::Error for the whole project, I meant wherever we deal with CompilerError (i.e., recoverable errors).

Ideally, we can even forbid unwraps on the crate level and use explicit #[allow... where unwraps are necessary.

That's extreme and may make it very difficult at a lot of places. Definitely not for Options, but if only for Errors, then maybe at some point. But right now I had only our current uses (and the ones in your PR) of CompilerError in mind.

[vaivaswatha]

I've been looking at MLIR's pattern rewriter infrastructure, and the whole thing (how it works, what it provides, and how it provides that - the API) is quite complex.

The rewrite drivers (greedy / dialect conversion etc) collect the operations and then the applicator applies the max benefit pattern. The rewrite drivers, if I understand correctly, in a way is provided by the walker in your PR.

Before we attempt to implement this complex system, do you think it's a good idea to try and write a conversion manually? See what difficulties, repetitions we face and then see how a system to "automate" that can be designed? If you can point me to your code which needs conversion, I can try playing with it too. That'll give us more clarity.

Ideally I'd want something much simpler than what MLIR has. Like maybe just a walker that allows modifications of the graph during the walk. But if we don't have anything good, we can go for what you're attempting - the MLIR way.

Thoughts?

btw, are you on a timeline / deadline by when you want something done or are more free to explore solutions?

[greenhat]

I've been looking at MLIR's pattern rewriter infrastructure, and the whole thing (how it works, what it provides, and how it provides that - the API) is quite complex.

The rewrite drivers (greedy / dialect conversion etc) collect the operations and then the applicator applies the max benefit pattern. The rewrite drivers, if I understand correctly, in a way is provided by the walker in your PR.

Before we attempt to implement this complex system, do you think it's a good idea to try and write a conversion manually? See what difficulties, repetitions we face and then see how a system to "automate" that can be designed? If you can point me to your code which needs conversion, I can try playing with it too. That'll give us more clarity.

Ideally I'd want something much simpler than what MLIR has. Like maybe just a walker that allows modifications of the graph during the walk. But if we don't have anything good, we can go for what you're attempting - the MLIR way.

Thoughts?

My initial idea was to take a simplified version of MLIR infra and use it as a starting point. However, during this journey, I scratched my head many times, trying to understand the MLIR's design but plowed on. So, yes, it's pretty complex, and we might not need it as is.
I'm only beginning to implement my conversions, and I'm unsure how much of the MLIR infra I'll actually need. I agree with you to keep it as simple as possible. For now, I see "match and rewrite" and legalization parts of it as quite useful, even at this early stage. As I progress with my conversions in this PR, I'll better understand what I found useful and ditch the rest.

Here is the code where I'm implementing the conversions - https://github.com/greenhat/omnizk/blob/add-miden-new-ozkir/crates/ir-transform/src/miden/lowering.rs

btw, are you on a timeline / deadline by when you want something done or are more free to explore solutions?

I'm not on a deadline, it's my side project, and I'm free to explore solutions.

What is your vision for error handling? I see that errors are used only in Verify trait with unwraps and asserts everywhere else.

The way I see it, unwraps and asserts, when they fail means that it's a bug in the core infrastructure. It shouldn't happen at all (ideally).

Recoverable errors are for when it isn't a bug in pliron. For example, something's wrong with the input program that it fails verification, or there's a syntax error in parsing etc. Conversion errors could be in that category too, i.e., not a compiler bug.

This makes total sense. I agree.

When I suggested that we do thiserror and anyhow::Error for the whole project, I meant wherever we deal with CompilerError (i.e., recoverable errors).

Will do.

Ideally, we can even forbid unwraps on the crate level and use explicit #[allow... where unwraps are necessary.

That's extreme and may make it very difficult at a lot of places. Definitely not for Options, but if only for Errors, then maybe at some point. But right now I had only our current uses (and the ones in your PR) of CompilerError in mind.

I agree. It's quite extreme, and I choose the wording ("Ideally") poorly. :)

[greenhat]

Hi! I was trying to check if an op is erased from the arena and found nothing, so I added Ptr::is_alive (not a fan of this name). I also make try_deref* methods to check if the pointee is still in the arena. Besides, I don't like Ptr::is_linked crash if the pointee is not in the arena. Do you think we should check there as well?

Here is my progress so far in OmniZK in using pass infra - https://github.com/greenhat/omnizk/blob/add-miden-new-ozkir/crates/ir-transform/src/miden/lowering/cf_lowering.rs

[greenhat]

@vaivaswatha ☝️

[vaivaswatha]

Hi! I was trying to check if an op is erased from the arena and found nothing, so I added Ptr::is_alive (not a fan of this name). I also make try_deref* methods to check if the pointee is still in the arena. Besides, I don't like Ptr::is_linked crash if the pointee is not in the arena. Do you think we should check there as well?

Here is my progress so far in OmniZK in using pass infra - https://github.com/greenhat/omnizk/blob/add-miden-new-ozkir/crates/ir-transform/src/miden/lowering/cf_lowering.rs

Sorry I somehow missed this last week. Just saw it after your ping today.

I'm curious, in what situation is Ptr::is_alive useful? In other words, I'm trying to see why having a Ptr to an erased entity isn't always a bug. (It's something like a dangling pointer, but safe as it'll panic if dereferenced).

[greenhat]

I'm using it at https://github.com/vaivaswatha/pliron/pull/4/files#diff-bb35f6399df11ed17d92ad8c06ec15cdeb5bfd973838cdedae6d0b593c1b2df1R238 when I'm iterating over the collected(walk) ops to check if the op was not erased by the pass ran on the previous ops.

[vaivaswatha]

I'm using it at https://github.com/vaivaswatha/pliron/pull/4/files#diff-bb35f6399df11ed17d92ad8c06ec15cdeb5bfd973838cdedae6d0b593c1b2df1R238 when I'm iterating over the collected(walk) ops to check if the op was not erased by the pass ran on the previous ops.

Isn't it then the Rewriter's responsibility to keep track of erased Ops (since it's notified about it anyway as the erase happens via the rewriter), and check against that before trying to do something else.

Somehow it seems to me that having dangling pointers is a bad idea and checking if it's a dangling one is just a patch / workaround and not a solution.

Having said that, if this is important for you (ie no other way of doing it otherwise), I'm ok with it.

[greenhat]

If I squint enough, I can see that both (interior mutability vs. pointer validity) errors might manifest only in certain conditions depending on the IR that is being processed. That means that on some inputs, my compilation pipeline hard crashes, and in case of pointer validity, I have no way to handle it gracefully.

[vaivaswatha]

When iterating over the IR during a transform, it's possible that we arrive at an instruction (Operation) the second time (when it's already borrowed mutably). For such algorithms, this is useful. It isn't an error, but is how the algorithm works. Although I admit, an argument can be made that it can be handled by a check similar to the erased_ops.contains() one we discussed y'day.

That means that on some inputs

I wouldn't say that it's a function of the input. If it crashes due to a pointer validity, then it's a compiler bug. It isn't something that you attempt to handle post-access at all. There shouldn't have existed a dangling pointer in the first place.

Do you have a requirement for this now? That try_deref not panic on dangling pointers? (I'm assuming that you already added the erased_op check in the conversions). If yes, then we can see why that situation arises and take a call. Otherwise, for now let dangling pointers be a panic. We can always add it back if there's a real need for the existence of dangling pointers.

[greenhat]

Do you have a requirement for this now? That try_deref not panic on dangling pointers? (I'm assuming that you already added the erased_op check in the conversions). If yes, then we can see why that situation arises and take a call.

No. erased_op tracking list solved that case.

Otherwise, for now let dangling pointers be a panic. We can always add it back if there's a real need for the existence of dangling pointers.

Ok. I will restore the original behavior and use expect to explain the error.

[vaivaswatha]

That sounds fair.

[greenhat]

Done in 733255e