review comments, test, lint

Generates this message:

shell
benoitjacob@cloud:~/mlir-build$ cmake --build . --target mlir-opt && bin/mlir-opt /tmp/x.mlir
[6/6] Linking CXX executable bin/mlir-opt
/tmp/x.mlir:2:13: error: 'vector.contract' op expected all input dimensions to be used by either the LHS or the RHS.
  %result = vector.contract {
            ^
/tmp/x.mlir:2:13: note: see current operation: %0 = "vector.contract"(%arg0, %arg1, %arg2) {indexing_maps = [affine_map<(d0, d1, d2) -> (d0, d2)>, affine_map<(d0, d1, d2) -> (d2)>, affine_map<(d0, d1, d2) -> (d1)>], iterator_types = ["reduction", "parallel", "reduction"], kind = #vector.kind<add>} : (vector<1x2xi32>, vector<2xi32>, vector<1xi32>) -> vector<1xi32>

better message

more review comments

Addressed review comments.

addressed review comments

Is this related to https://github.com/llvm/llvm-project/issues/54469 ?

The above 2 lines really were a full testcase -- just needed to wrap that in a function!

OK, I have dug deeper now :-)

In D122026#3393235, @nikic wrote:

This looks like a consistency assertion that an <n x %ty> vector has an n-element scatter in the cache -- so it's not obvious to me that just dropping the assertion is fine, I think you might be hiding a legitimate issue here.

I don't have write access. Can someone please push this for me?

review comments

rebase

rebased

I don't have write access to the repository. Can someone push this for me?

give the cl opts precedence. per review comment

clang-format

fixes

Context: I needed this for https://github.com/google/iree/pull/8474 . I found that TSan instrumentation expects vector sizes to be <= 16, and in my project (IREE) we have tests with higher vector sizes. That left some tests function uninstrumented, resulting in crashes as instrumented called into them.

Thanks for the quick reply! Great to hear this is going to be resolved in the principled way. I'll switch to fixing the faulty call sites in IREE, then.

clarify comment

FYI @benvanik @GMNGeoffrey @mravishankar

For what it's worth: the new home of this is https://github.com/google/iree/pull/8409

Abandoning: per discussion with @mravishankar , quantized_matmul may be an oddity not worthy of a new MLIR core pattern. Going to take this to IREE for now. Feel free to reopen / take over this if there is interest in this on the MLIR core side.

remove unneeded using namespace

Ready for review!

In D119202#3303350, @ThomasRaoux wrote:

In D119202#3303347, @Benoit wrote:

In D119202#3303339, @ThomasRaoux wrote:

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

Better ask @nicolasvasilache ! In https://reviews.llvm.org/D114993 I had originally written a larger number of tests. Nicolas took over and disabled/removed tests.

ok, what about the failure condition, why do we have to care about the memref rank?

In D119202#3303339, @ThomasRaoux wrote:

@Benoit could you take a look as I believe you added those patterns. Why doesn't the memref has to be rank 1 for the pattern to skip? Do you know why the unit tests above are disaled?

rebased and confirmed it is still needed to use vector transfer flattening.

I don't have permissions to submit this myself.

rebased

rebased

Thanks for the review and explanation. I have updated the code to apply your inline comment i.e. restrict the change to the static shape case.
I haven't updated docs as you suggested because I'm out of my depth here, sorry.

restrict to static shape case

Hi Nicolas, thanks for the kind supportive words here regarding the usefulness of these patterns/helpers.

More review comments.

Apply Mahesh's suggestion to use inferRankReducedResultType.

more review comments

split into 2 patterns

apply some review comments

Thank you all for weighing in. After discussion with @nicolasvasilache and @ThomasRaoux today I switch to dropping the unit dims, which caused my need for the present diff, by inserting rank-reducing memref.subview in the pattern that is what generating the problematic memref.collapse_shape, see https://reviews.llvm.org/D114993 . So I can now close this diff, but thanks all for the conversation.

Thanks for the review comments! I'll apply them on monday. For now I've just updated this diff with the new idea to use rank reducing subviews to drop unit dims, (thanks @ThomasRaoux for the suggestion), that removes my need for https://reviews.llvm.org/D114821 so I can drop it now.

add rank reducing subview to drop unit dims

In D115019#3171261, @mehdi_amini wrote:

Sorry! Pushed a fix in b28f317c8156 and will monitor the bot tonight.

This should be good to submit now.

fix preexisting race condition in isWorkerThread found by MLIR tests with TSan

Wait a bit more before submitting... we're still debugging CI failures.

Rebase

In D115019#3168875, @mehdi_amini wrote:

LG, thanks!

review comment

In D115019#3168840, @mehdi_amini wrote:

I'm fairly sure I fixed this one or two months ago, at least the C++ API allows to setup a project without starting a ThreadPool at all. This was a bottleneck in TensorFlow for some small MLIR work we're doing there.

There might be some similar plumbing we could do in how we handle --mlir-disable-threading with mlir-opt as well, since it is a testing tool we haven't tried to "optimize" this kind of things (you're making a good case for it though!).

Address review comments.

https://reviews.llvm.org/D114702 landed a few days ago and fixes the dim vs idx issue that I was reporting here! Thanks @herhut !

Got it, getThreadCount() was really meant to return the max number of threads, not the current number. Those two numbers used to be the same before this patch.

off by one mistake in max number of worker threads

newline at end of file; minor wording update

Rename the test function - was not match what it actually did