Adding Thomas and Christian to see whether they have comments wrt. the GPU operations.

Thanks for cleaning this up.

Thanks!

In D95041#2510841, @mehdi_amini wrote:

Since the split of the standard dialect has begun, can you create a math dialect and move this kind of operations there?

Hmm, this turned out more complex than I had thought. I had a simple push/pop in mind. If that is not enough, lets keep it at the simple version for now.

Thanks for cleaning this up!

Thanks for the explanations!

Please fix comments.

Thanks!

This still seems a bit like a hack to me but it is better than twiddling with runtime paths.

Thanks for adding these. Beyond coding style this looks great!

I don't mind adding this generic default cast but I hope dialect conversion will remain configurable as to which cast is being used.

Nice cleanup, thanks!

This also adds the lowering to LLVM. Maybe mention this in the commit message or split it out.

Could you wordwrap this to make it easier to read?

Thanks for fixing these. Just some remarks while looking through the changes.

Thanks.

Thank you! Big improvement in naming.

Thanks for cleaning this up!

So, to conclude here, it seems to me the best way forward is to move this dialect conversion ops to their own dialect and make it clear that they are essentially cast operations between tensor and memref for the purpose of conversion. This is a discussion for discourse, though.

Thanks!

In D92562#2446427, @silvas wrote:

In D92562#2444990, @herhut wrote:

The issue with free can be solved by first inserting frees and then rewriting them, which is what we would do today. The issue with copy is more complex, as it currently does not introduce a single operation. To enable the latter, we have discussed to always insert a copy operation and then users can rewrite them at will. I think there is a forthcoming post from @dfki-mako about that. The idea essentially would be to have a bufferize dialect, that contains copy, cast and maybe free operations that can have semantics that are tailored to the needs of bufferization.

The current approach assumes that all memrefs that this pass will attempt to free can be freed/cloned by the ops produced by these callbacks. That seems incredibly fragile, considering what you said about bufferization patterns being able to freely choose their allocation operation -- I think that's an incredibly strong argument that we need a more general solution, probably something that rationalizes the invariants needed by this pass with what bufferization patterns are allowed to do. It seems that we would need to require all allocating ops to specify what resource/tag they allocate from so we can know how to free it, and then do some dataflow analysis to propagate that information, and fail BufferDeallocation if we cannot figure out which clone/free is legal to insert.

lgtm with extra test.

Thanks!

There should be some test that captures this.

In D92562#2443647, @silvas wrote:

Having this customizable with a callback will allow users to potentially start doing all sorts of random stuff inside. What if a user starts depending on that? For example, what if the callback references some side data structure and generates different clone/free ops depending on that, possibly in a less structured way than we would like to support upstream. That suddenly becomes much more difficult to unwind. Of course, implementing that callback is likely to be done in a downstream project, where doing something like this as a shortcut might seem more acceptable. But that shortcut then becomes a cost that the MLIR community needs to pay to unwind!

First, the work @silvas did on improving how bufferization works in MLIR core brought a great cleanup with it and the outcome is much nicer than what we had before. Thank you for investing your time and thought on it. A lot of what got cleaned up had to do with how operations get bufferized and how we can do this partially and incrementally, a functionality that we did not have before and that we are already heavily using now. Awesome!

This seems to do the right thing, I just don't understand why.

Can you add a test?

I disagree with @silvas on this. This particular change does not take any opinion on the discussion you reference. It merely makes the hard-wired current behavior configurable by choosing different ops. It does not mandate how the decision which free operation to use is done.

Thanks!

Thanks, much clearer now. I am confused a bit by the types that get used but otherwise this looks great.

Cleaning this up in a follow-on is fine too. Please let @ftynse comment, too.

Fix test

PTAL. This fixes a use after free.

Clean up test invocation

I realized that we cannot rewrite all ReturnLike operations, as they require rewriting of their parent which cannot be done generically. PTAL the changes.

Do not rewrite ReturnLike ops in general.

Thanks for the reviews!

Comments

Thank you for cleaning this up. I was not aware that the llvm conversion does not have Op specific patterns, yet.

Frederik, can you take a look? Sean was happy for this to move forward with his comments addressed, which I did.

Split out finalizing pass.

PTAL.

Rework to support terminators that implement interfaces.

Thanks. Just some nits.

I am fine either way, just a suggestion.

I assumed these operations are in the same category as the LLVM::DialectCastOp (also known as llvm.mlir.cast). There it says

Thank you!

Nice, thanks!

Thanks for the reviews!

Comments