@MaheshRavishankar -- is there a risk that this op will be optimized worse due to not being recognized as well as the regular depthwise?

In D127225#3565062, @gpetters94 wrote:

I think it makes more sense for it to be major, if only for legibility's sake.

I suspect the g dimension should be contiguous with the C dimension, and I'm not sure if it should be major or minor to it.

This direction makes sense to me. I am a little out of the loop from the current bufferization interfaces, so I will let someone else review the code in detail.

Can you add a pass / reusable pattern that converts ml_program.func to func.func and the reverse? Otherwise I feel like we are going to roll that many times downstream just for our initial push in IREE (can be a separate patch of course).

Nice :)

You can see my full code here: https://github.com/silvasean/iree/commit/cba1cf90fa8e8cc9b3bdacfd034f7d53c40661ad

Hey River, I was trying to use this downstream and it isn't working for me

Thanks for fixing this River :)

Thanks River!

Left a comment there.

In D122142#3396788, @zero9178 wrote:

In D122142#3396703, @lattner wrote:

Thank you for tackling this! I agree that "OperandType" isn't the right thing, but I don't think "OperandReference" is either. The key thing about this is that it models an occurrence of an SSA value in an operand position, but without having a type applied to it. "Operand" and "Reference" are somewhat redundant.

What do you think about a name like "UnboundOperand" or "OperandWithoutAType" (joking but may you have a better idea) or OperandSyntax or LexicalOperand?

Hmm. One of the names I had in mind as well was "OperandOccurrence". From the names you listed I like "UnboundOperand" the most (bit of a symmetry with resolveOperands as well).
I don't have a very strong opinion of any of those names however (except maybe "OperandWithoutAType" :) ).

nice :)

The idea here of preventing invalid ops makes sense to me, but @herhut probably is more knowledgeable about this code for a detailed review.

Nice :) This looks pretty straightforward, but given how core this is, wait for another approver or two.

Yay!!

In D111620#3104504, @rriddle wrote:

In D111620#3104485, @benvanik wrote:

This change broke 1->N type conversion (as noted in a FIXME in here). This is really unfortunate and there doesn't seem to be any workaround now that the materialization is deferred and the casts are inserted in a way that doesn't allow for patterns to match them. Is this something that can be rolled back or fixed soon? 1->N type conversion during dialect conversion did work before this change so this is a regression.

The FIXME is more of further elaborating on the existing state, it was not some new problem added by this patch. Can you file a bug report with what your exact situation is?

This looks good to me. @rriddle @dexonsmith @dblaikie for thoughts.

In D112736#3094966, @ezhulenev wrote:

Basically add a pass something like X86vectorMathApproximationsPass?

1. I would just like Math dialect transforms to not depend on target-specific dialects. So concretely just RsqrtOpApproximation pattern would move to x86vector/Transform/, since it is the target specific one.
2. I agree, removing Polynomial would be good.

Can we move this RsqrtOp lowering to the x86vector dialect? It seems really out of place in the math dialect, which should remain target agnostic. (also, since this approximation is newton-raphson based instead of polynomial based, it seems out of place in PolynomialApproximation.cpp as well)

This doesn't seem to be the right pass to do target-specific lowerings. Can you add math.rsqrt and move the vector unrolling to a generic pass in the vector dialect? Then this pass wouldn't depend on target-details.

Glad to see this happening!

nice!

LGTM. Probably wait for another pair of eyes to LGTM it. @ezhulenev perhaps?

Thanks!!!

I think it's fine for const_shape to always be static and require a tensor.cast to erase the static shape to dynamic.

Thanks!

+1 to Jacques suggestion to listing some prior art here. In the project ideally or in the industry as well would be good.

In D110425#3033028, @ftynse wrote:

What would be a more correct phrasing for the semantics description in your opinion? We want the op to always result in a new value.

I don't really understand the argument about the operation having to be always folded away. This would effectively render impossible any clone or copy operation. Yet, there is a TensorCloneOp in IREE if I remember correctly and it isn't an always-folded noop. So there is clearly precedent and a use case for operations defining new values with same "content" as the existing values.

The additional attribute may indeed be interpreted as preventing some canonicalization wrt the semantics of the original op (or its version with the attribute unset). However, I argue that we just choose to define different canonical forms for the op depending on it having the attribute. Referring to the post our doc on canonicalization quotes in the opening paragraph - https://sunfishcode.github.io/blog/2018/10/22/Canonicalization.html - that describes how canonicalization and redundancy elimination get ambiguous: "<...> ultimately, in its purest form, canonicalization just focuses on removing unnecessary variation so that subsequent optimizations can be simpler", this change is exactly about removing, or rather shifting, variation to simplify subsequent optimization. There exist subsequent optimizations such as hoisting that are rendered significantly simpler by always having a fresh value that is always defined by pad_tensor.

@herhut - thoughts on keeping scalars (i1) as disallowed from Same*Shape traits?

Personally I think we should deprecate this behavior. In my mind, an i1 doesn't have a shape at all. So it doesn't make sense to say it has the same shape as anything else. Is there a specific use case for this behavior (IIRC this might have fallen out of an implementation detail of an earlier implementation of the trait).

Specifically, it is impossible for these two functions to have different semantics (as long as linalg.pad_tensor is marked NoSideEffect) -- both return a value that is identical to their argument, and it is always legal to fold @with_pad to @without_pad per tensor value semantics:

In D110425#3029615, @nicolasvasilache wrote:

In D110425#3028927, @silvas wrote:

Sorry I didn't see this earlier, but I don't think this direction makes sense. This op operates on tensors, which are value semantic, so terminology like "guaranteed to create a new tensor suitable for packing" simply don't make sense.

I fail to see what doesn't make sense? Packing is done on tensor semantics too: this can be also called "tensor tiling" by opposition to "op tiling".

Having an attribute that prevents folding from happening gives me bad memories of e.g. tf.Identity being marked as having side effects to prevent folding. I would rather not go down that route.

I do not see the relation between the 2.
Marking tf.Identity as having side effects is clearly a hack misusing side effects for other purposes: the op still has no side effects.
Here, the semantics of the op are : it is illegitimate to fold this op. There is no discussion about side effects or any other interface.

Sorry I didn't see this earlier, but I don't think this direction makes sense. This op operates on tensors, which are value semantic, so terminology like "guaranteed to create a new tensor suitable for packing" simply don't make sense.

address

did a pass. looks good to me. probably want to wait for a handful of LGTM's given the far-reaching consequences of this change. Ideally both River and Chris could weigh in.

Nice!

Awesome! Thanks!!!

Thanks Alex!

Thanks Alex!

Can you please revert this commit and the previous one I objected to? I think they are fundamentally going in the wrong direction.

Can you please add the integration test cases I suggested in the other patch? I'm still pretty sure this will miscompile. Fundamentally you are writing this as an "in-place" bufferization and that cannot be done with the current framework because it involves non-local reasoning.

It feels like this bufferization pattern would miscompile if we were to write integration tests analogous to these:

Beautiful :)

great work yi!

Thanks!

I don't think we need an integration test here. A regular IR test would be enough to exercise it? (just snapshot the IR from the itnegration test before linalg-bufferize)

Do you know why previous tests didn't need this? Should we add a new test?

Please add lowering to LLVM for this too (doesn't have to be this patch precisely, but a fast follow-on please).

Will let @sjarus review this as the detailed shape transfer function review is best done by someone deeply aware of the spec.

reword