User Details
- User Since
- Mar 8 2021, 11:11 AM (68 w, 3 d)
Yesterday
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
Wed, Jun 29
more review comments
Addressed review comments.
addressed review comments
Tue, Jun 28
Apr 13 2022
Is this related to https://github.com/llvm/llvm-project/issues/54469 ?
Mar 20 2022
The above 2 lines really were a full testcase -- just needed to wrap that in a function!
Mar 19 2022
OK, I have dug deeper now :-)
Mar 18 2022
Mar 10 2022
I don't have write access. Can someone please push this for me?
review comments
Mar 9 2022
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
Mar 8 2022
clang-format
fixes
Mar 7 2022
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.
Mar 3 2022
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
Feb 24 2022
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!
Feb 22 2022
Feb 8 2022
Feb 7 2022
Jan 13 2022
rebased and confirmed it is still needed to use vector transfer flattening.
Dec 15 2021
I don't have permissions to submit this myself.
Dec 14 2021
rebased
Dec 13 2021
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
Dec 12 2021
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.
Dec 10 2021
more review comments
split into 2 patterns
apply some review comments
Dec 3 2021
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
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
Dec 2 2021
review comment
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
Dec 1 2021
newline at end of file; minor wording update
Nov 30 2021
Rename the test function - was not match what it actually did