Fix variable name.

fix typo

+1, added some verbiage to that effect

Address comments.

In D87492#2267795, @jpienaar wrote:

The SameOperandsAndResultElementType is redundant with the new

TypesMatchWith and prevented having zero elements.

So is it only conceptually redudant but the verification differs or is it really redundant? And why is zero elements different?

LGTM from my side

In D86392#2250881, @mehdi_amini wrote:

In D86392#2250855, @silvas wrote:

I still consider this feature naming (such as test case naming) very confusing.

But it seems like you also never encountered "pipeline" which is surprising to me as it seems like a very common terminology (and we've been using it in MLIR "forever").

I still consider this feature naming (such as test case naming) very confusing. It's actually very simple to explain: "it allows a pass to call other passes". The name "dynamic pipeline" is very confusing -- like I'll need to re-explain to every new MLIR user that encounters the feature what the feature really is. "user: How can I call a pass from within my pass? mlir community: oh, use our dynamic pipeline feature. user: what is that? mlir community: it's a way to call a pass from within another pass; user: oh".

In D86392#2248436, @mehdi_amini wrote:

In D86392#2247937, @silvas wrote:

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

I still see this as dynamic: from the pass pipeline point of view there is a "pass" which will execute "dynamically" sub-pass-pipelines. This feature enables dynamism in the pass pipeline where it does not exist today.

Now that this functionality has evolved, is there a better name to describe this functionality than "dynamic pipeline"? It's not associated with pipelines in any specific way, and it is not associated with dynamism per se (e.g. inliner could use a statically hardcoded simplification pipeline). Perhaps "recursive pass invocation"?

Using #ifdef's isn't great. Can we instead have a cl::opt and just use a regular if to control this. Then we can test it too.

Also, can you update the commit description to include a description of the namespace changes?

Is there anything about this specific to index type? Can this issue happen with any user-defined type?

Can't we use shape.concat for this?

What do you think about

%1 = shape.const_shape [1, 2, 3]
%2 = shape.const_extent_tensor [4, 5, 6]

instead of

%1 = shape.const_shape [1, 2, 3] : !shape.shape
%2 = shape.const_shape [4, 5, 6] : tensor<?xindex>

?

Thanks for adding this!

It's kind of confusing that this is in llvm/utils while generate-test-checks.py is in mlir/utils. Is there a plan to reconcile this difference?

LGTM modulo incorporating our decision on the error case discussion from shape_eq.

LGTM modulo getting feedback for others on how to define the error cases.

Thanks!

It would be good to update https://mlir.llvm.org/docs/Dialects/LLVM/

LGTM, modulo splitting into two ops and a nit.

(we could also use shape.eq to be the one for shapes, which is a bit shorter than shape.shape_eq; I personally like being a bit more explicit but it's up to you what feels best namingwise)

LGTM modulo splitting into two separate ops.

Great! Thanks for these nicely factored patches!

Thanks!

Awesome, thanks!

LGTM modulo renaming of op (discussed in the dependent patch)

As per discussion today, this pattern is going to "fight" with what we are doing with IREE (and also npcomp). Can we reserve this pattern for the shape to std-with-descriptors lowering?

Can you add documentation for what happens in the case where the dimension index is out of bounds? Are we allowed to assume that doesn't happen?

What is the invariant established by this pass that other passes can rely on? It seems that this pass is needed for correctness in many cases, not just optimization. So it needs to have a well-specified contract for later passes to rely on.

Thanks! For some reason I was assuming we already had this, so seeing it turned on is great!

Thanks!!!

mlir/Dialect/Vector/CMakeLists.txt doesn't seem to need this?

@tpopp can you PTAL at this? Surely we aren't the first using DRR inside mlir core, and I didn't find evidence that this CMakeLists.txt annotation was needed for them. Wild guess, but perhaps we can put the ShapeCanonicalization.td in lib/ ?

LGTM with one requested modification and a nit.