Nice cleanup and fixes. Thanks!

Thanks!

Looks good modulo disabling multithreading. I assume that is a left-over from debugging?

Fix comments and re-enable cast handling

Thanks!

Fix spelling

Hey @dfki-jugr. I debugged this a bit today and here is how far I got. We can either land this as a cleanup or you can fold it into some changes you want to do anyway.

Sent the wrong state of the change

I think this should become a pass instead, so that it can also consider aliases and
remove more clones. Block local aliases would suffice to catch situations like

Fix Cmake files.

Minor cleanup

fix tests

There is a typo in the description. Otherwise lgtm.

Thank you for cleaning this up!

I am +1 on the direction here.

Thanks for the replies.

In D100838#2741904, @tpopp wrote:

I've worked around this issue in my own code now, so what do you think about me just doing this rename? Then there will be no confusion between this pass and the new AliasAnalysis (and their definitions of aliasing). I think that confusion is good to avoid, and since I have a different solution for my own use case, I can avoid creating more confusion by creating another Analysis at this time.

Thanks!

On what op is this used as there is no test here?

In D101749#2733073, @ftynse wrote:

I am missing the rationale for this one. Maybe "roundtrip" is a bad name, but we have these tests for all dialects and the point has always been to check that MLIR prints what you expect. Without the checks, you only test that MLIR can parse back what it printed, but you lose the consistency check. Back when we had to write parsers manually, I remember having caught several cases where an ill-written parser would accept A and B keywords, but construct an op always with A; so it would be printed with A instead of B and parsed back successfully (thus passing the trimmed-down test) but would contain an error.

I am not convinced that DependencyAnalysis is the better name here. For dependencies, I think of read-after-write, etc.. This only computes the flow of buffers through control flow.

Thanks. I can see this to be useful as we use -1 for unknown quite often.

Nice. Some comments to further generalize.

In D99172#2670712, @bondhugula wrote:

@dfki-jugr But why does this revision remove the copy removal pass when there are things that generate copies? The extension to canonicalization done eliminates clone ops, but IR with copy ops will no longer have a way to remove copies? For eg. -linalg-bufferize would still generate copy operations, right? @silvas

In D98041#2648155, @silvas wrote:

I think just a patch splitting the op into tensor.dim and memref.dim would be fine. There isn't much to discuss.

LGTM for the direction this takes. Please address nits.

Thanks for cleaning this up.

Nice!

This moves us a little further in the direction of having more precise types in the shape dialect in the non-error bearing case. I think this is the right direction to go but also want to give Jacques a chance to comment.

Generally looks good. Just one question. Thanks for adding support for this!

I think we should land this as it is strictly an improvement over the current documentation. We can improve further but having it documented avoids more confusion than it creates.

Thanks!

Review comments...

Thanks for cleaning this up! Looks good in general but I also cannot test this. It would be nice if we had a builder for this. @whchung do you have a setup that could test this continuously or at least periodically to make sure it does not break?

In D97887#2618054, @mravishankar wrote:

W.R.T your comment about using OpFoldResult, I think it comes down to what you are using the interface for. To me it just felt more uniform to get the shape of all the dimensions. There is no need to check on the analysis side to check if the shape is dynamic and then go query the interface. You query the interface and it gives you the entire shape (static and dynamic) and then its a matter of just extracting the Value (or int64_t if you can use that) on the client side. Much cleaner usage AFAICS.

Fantastic! Thank you for cleaning this up.

Please address the clang tidy warnings. Otherwise this looks good to go to me.

Thanks!

After off-line discussion that this will be morphed into the final state in multiple changes, this looks like a good first step. Please fix the comment, though.

Thanks!

Wow, what a patch :)

Thanks!

Nice find!

Thank you @ftynse and @bondhugula for your many comments on this patch and thanks @navdeepkk for addressing them!

So we get to use the context stack after all. Looks very clean now.

Just nits for comments. Looks good, thanks!

Thanks!

lgtm for a first step in making the pass pipeline more configurable.

In D96776#2565842, @ftynse wrote:

I won't ask to split out the math conversion from "standard" conversion, yet, but have you considered implementing this (as well as rsqrt from which most of the code is duplicated) as a sort of expand-math conversion instead?

Thanks. Phabricator keeps hiding these from me (might be my fault) and I have to find them in my email.

This looks good. Is there a corresponding diff that adds the expm1 operation to math?

In D96739#2564409, @mehdi_amini wrote:

I don't think a pass is the right way to model this, if we want to have support for fast-math we should model it through ops on each attributes, on the model of what LLVM is doing.

+1 to landing and then cleaning up tests.

Implement inlining interface

Thanks for the review!

Added missing tests

Fix formatting issues.