Thanks you Matthias for doing this! I apologize for leaving this in the sorry state that it was left in.

@HassanElDesouky sorry, I'm not that familiar with this code.

This makes me so sad that the memref.dim infection is forcing this to go in memref/transforms :'(

Hi @dfki-jugr, can we please split memref.dim into tensor.dim (and memref.rank into tensor.rank). I've now independently heard complaints about this from 5+ people across many different teams I interact with.

DimOp should already have a canonicalizer that will use InferShapedTypeOpInterface to rewrite dim(linalgop(x)) to some_expr_of(dim(x)). Do you have a test case where that doesn't happen?

LG once updated to not use the interface.

Nicolas and I had a long conversation regarding the "in place op interface". We managed to tease out two separate concepts that are independent.

Code looks fine. Will let @sjarus review the rounding algorithm details.

It seems there is agreement that this is a useful feature to support. Can someone LGTM?

In D102200#2750906, @jpienaar wrote:

In D102200#2750867, @silvas wrote:

In D102200#2749475, @mehdi_amini wrote:

In D102200#2749288, @silvas wrote:

In D102200#2749139, @mehdi_amini wrote:

I don't quire get why can't just use the dialect namespace?
Anyway, not really opposed either, but can you add an op in the test dialect that would use this?

What I want is for different ops in the dialect to be in different namespaces. See dialect.td in the patch for an example.

I understand what the patch is doing and what you want, I don't really understand why...

See my above comment:

I have a dialect in npcomp that would greatly benefit from this. The basic problem is that we are importing a notion of "op" that is present in PyTorch, and has a "namespace" and "unqualified portion" of the op names. The "namespace" isn't really a dialect, but is useful to map to C++ namespaces in the code. We still model all these ops as part of a single dialect of "registered torch ops".

The basic problem is that PyTorch has notions of "ops" like prim::ListUnpack, aten::add, etc. The logical thing (because these "namespaces" are not "dialects" in the MLIR sense; they are just random prefixes that don't really have a strong structure to them) is to import them into the same dialect as torch.prim.ListUnpack and torch.aten.add. However, at the C++ level, I still want to retain ...::prim::ListUnpackOp and ...::aten::AddOp. Does that make sense? I could instead do PrimListUnpackOp and AtenAddOp but it just seems ugly.

I'd have thought it most natural to import each into its own dialect (e.g., torch_prim.ListUnpack) or having "subdialect" concept (which has been tossed around but unclear what it means - although with dialect fallbacks these become interesting because a subdialect could fallback to main dialect ...). These seem to be in same dialect just because from same frontend?

In D102200#2749475, @mehdi_amini wrote:

In D102200#2749288, @silvas wrote:

In D102200#2749139, @mehdi_amini wrote:

I don't quire get why can't just use the dialect namespace?
Anyway, not really opposed either, but can you add an op in the test dialect that would use this?

What I want is for different ops in the dialect to be in different namespaces. See dialect.td in the patch for an example.

I understand what the patch is doing and what you want, I don't really understand why...

factor helper

In D102200#2749139, @mehdi_amini wrote:

I don't quire get why can't just use the dialect namespace?
Anyway, not really opposed either, but can you add an op in the test dialect that would use this?

I have a dialect in npcomp that would greatly benefit from this. The basic problem is that we are importing a notion of "op" that is present in PyTorch, and has a "namespace" and "unqualified portion" of the op names. The "namespace" isn't really a dialect, but is useful to map to C++ namespaces in the code. We still model all these ops as part of a single dialect of "registered torch ops".

We should commit this, but it is unfortunate, because now we have a dependency cycle between std and memref, e.g. MemRefDialect::materializeConstant uses mlir::ConstantOp (actually, it seems like a bug that we don't have a dependentDialects for the memref dialect depending on std)

btw, I think you might need to reupload the patch. I don't see the new code.

Any progress on splitting memref.dim into tensor.dim?

Seems fine to me in principle. Wait for others to weigh in though. @rriddle @dexonsmith

LGTM from me. Please get LGTM from someone with sparse compiler expertise on the exact API for SparseTensorEncodingAttr (choice of default, what would be most ergonomic, etc.).

I still caught a few places using bare "lattice" (noted some; more exist). There are a lot of cases where you say "lattice value" or "lattice state" as well which probably should all be normalized to "lattice element".

LGTM. The code looks reasonable, assuming you verified the outputs are correct on a few cases (always tricky to statically inspect the code).

In D101008#2709611, @aartbik wrote:

          SparseTensorEncoding: an API for a sparse tensor (nothing else), making it a first-class citizen; note that this is extended TACO style, so you have to trust us a bit that it has the right components 

I still would rather see this as just attributes on the SparseTensorAttr class, until there is a demonstrated need to make this an interface (and that this particular interface is the right choice). But I don't have the energy to argue it further. Carry on.

Thanks!

Awesome. Looks great!

Any progress on splitting memref.dim into tensor.dim? Multiple downstream projects are now complaining about this.

Thanks! Great work!

"encoding" SGTM.

In D99548#2680345, @mehdi_amini wrote:

properties LGTM as well.

This is not a correct xform because broadcastability is not transitive.

In D99172#2674514, @bondhugula wrote:

In D99172#2672229, @silvas wrote:

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

I believe all uses of copy in linalg-bufferize can be replaced with memref.clone easily -- it's a much more natural representation and we should do that cleanup.

Yes, but I wish -copy-removal was removed *after* this cleanup was done --- since there are now common copy patterns for which there is no longer anything in the infrastructure to eliminate/optimize.

Not sure if you want to add folders in this patch.

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#2650101, @herhut wrote:

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.

So there was a discussion that this could be done via interfaces, for example. We could have a generic dim and rank operation that can be applied to any ShapedType type. That would also make it easier to rewrite between these types, as the dim and rank operation would no longer need to change.

Just to be clear, I am not saying we should not do this split, I am just trying to avoid the extra work if soon later we decide that there is a different and better way to model this. This is more work than "just a patch", especially for downstream users, so we should avoid unnecessary churn. As far as I understand it, this is about an extra link time dependency that is not worse than the monolithic dialect before.

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

It looks like this patch simply moved std.dim to memref.dim, without doing the "splitting" which we had discussed. I now have code that only operates on tensors that is needing to pull in the memref dialect just for this. Can you split memref.dim into tensor.dim for the tensor case?

I haven't audited it yet, but I suspect we can do better. In this case, the binding for PassManager::run would need to check that the context for the op is the same as its context.