Comments...

In D87781#2282696, @bondhugula wrote:

It's a bit odd to see the loop ordering of a *lowering conversion* change because the output improves performance with a specific backend/compiler and for specific reasons. Moreover, that way, this would keep changing/evolving and be sensitive to common downstream paths and sensitive to LLVM's opt pipeline. In the absence of any target / scheduling info, the order to choose is typically expected to be just the most intuitive / canonical and shouldn't keep changing. You need an optimization mechanism if you need a better one.

I agree with you a lowering conversion shouldn't change because of specific perf/backend improvements. but this particular change take care of defaults (linear layouts, nhwc order...etc) so it sets the natural iteration order (n, h, w, kh, kw, ci, co) for most naive c++ multi array expression:
Y[n, h, w, co] += W[kh, kw, ci, co] * X[n, h + hk, w + kw, ci];

In D87781#2282696, @bondhugula wrote:

It's a bit odd to see the loop ordering of a *lowering conversion* change because the output improves performance with a specific backend/compiler and for specific reasons. Moreover, that way, this would keep changing/evolving and be sensitive to common downstream paths and sensitive to LLVM's opt pipeline. In the absence of any target / scheduling info, the order to choose is typically expected to be just the most intuitive / canonical and shouldn't keep changing. You need an optimization mechanism if you need a better one.

In D87781#2281813, @mravishankar wrote:

In D87781#2281741, @nicolasvasilache wrote:

Putting a blocker to get some discussion here.

The original definition was following TF, it is not anymore.
Do we consider this to not be a problem ?

Putting the loops in the "right" order is independent from the op spec.
You should be able to just use tiling without any op definition change (tile by 1-1-1-1-1 + perm for scalar loop-level).

More generally, the way I am approaching this longer term is that this magic "embed the world op" would disappear in favor of more, smaller named ops.
Transformations would look like: named_op1 -> generic form -> named_op2 and I think a lot of these things can be automated from the TC form.
My overall concern is whether we are going towards relying more and more on an op with very fat one-off semantics.

I won't oppose it strongly for now but the conv op will most likely be a prime candidate for deletion when the infra is more mature, so I'd be cautious on putting a lot of effort on transformations / optimizations / perf results if they don't generalize to linalg.generic.

Also, the doc would need to be updated too.

Actually a better way to have handled is to just lower it to generic form and do the interchange while lowering it, i.e. use the LinalgLoopInterchangePattern on linalg.conv operation. The issue with this AFAICS is

1. Since the indexing maps and iterator types are not mutable for named ops (as it should be) the linalg.conv needs to be lowered to its generic form while interchanging loops.
2. To do (1) there needs to be a way to get the body of the generic op. For the "legacy" named ops, these are hard-coded when they are lowered to loops. Instead the named ops need an interface that can be used to generate the body. If we had that, then I would favor the approach of keeping the conv definition as is and reordering the loops as a transformation.

In D87781#2281741, @nicolasvasilache wrote:

Putting a blocker to get some discussion here.

The original definition was following TF, it is not anymore.
Do we consider this to not be a problem ?

Which definition did we follow ? I am not familiar with explicit loop order def the only one I can see is the expression in linalg doc that doesn't imply the order, no ?

rebase.

In D86619#2249212, @nicolasvasilache wrote:

This is not how I was imagining convolution to be lowered in a first stab.
I was expecting 1. taking subviews, 2. either using reshape or extending subviews to allow it to be rank-reducing, 3. rewrite as a linalg.matmul and reuse the existing vectorization patterns.

This revision wants to introduce a vectorization pass with assumptions that look fishy to me.
I have left some comments assuming we're taking the route of direct vectorization of convolutions.

In any case we should have a VC to discuss these assumptions that I have not followed and how they differ from the existing work based on promotion + static shapes + vectorization + forwarding patterns.

Also + @asaadaldien who has started looking at similar things for depthwise convolutions on mobile.

In D81618#2086974, @ftynse wrote:

Is it possible to get this lowered to a tanh function call instead of the exp rewrite ?

Unless we assume libc is linked, there's no tanh function.

Didn't go through the code yet. high level comments:

LGTM!

resolve nit

@rriddle This already landed (not sure why not showing that here!) the example in the summary I will added it as a test in a follow up diff.

Resolve comments & clang-format

LGTM!

use dyn_cast_or_null

LGTM! clang-format + few nits...

LGTM!

LGTM! an inline question trying to understand the Builder blocks scoping here.

Resolve comments....

Thanks @nicolasvasilache

@jpienaar D72173 should fix that

applying clang-format

Resolve comments

lint fixes...

Resolve comments...