Page MenuHomePhabricator

[mlir][VectorOps] Implement strided_slice conversion
ClosedPublic

Authored by nicolasvasilache on Jan 6 2020, 6:54 PM.

Details

Summary

This diff implements the progressive lowering of strided_slice to either:

  1. extractelement + insertelement for the 1-D case
  2. extract + optional strided_slice + insert for the n-D case.

This combines properly with the other conversion patterns to lower all the way to LLVM.

Appropriate tests are added.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 6:54 PM

Unit tests: pass. 61127 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61127 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

andydavis1 accepted this revision.Jan 7 2020, 5:01 PM
andydavis1 added inline comments.
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
737

These will be merged with other CL, yes?

794

Same comment as other CL, comment on how recursion terminates.

This revision is now accepted and ready to land.Jan 7 2020, 5:02 PM
rriddle requested changes to this revision.Jan 7 2020, 5:03 PM
rriddle added inline comments.
mlir/include/mlir/IR/Attributes.h
1472

This needs more work. Will try to respond by end of day.

This revision now requires changes to proceed.Jan 7 2020, 5:03 PM
rriddle added inline comments.Jan 7 2020, 5:47 PM
mlir/include/mlir/IR/Attributes.h
1472

(sorry for the delay)

I would move the iterator, and anything related, to ArrayAttr itself. This is cluttering the global mlir namespace, which is something that we shouldn't do in general. I would imagine that we would have some 'get*<>' method on ArrayAttr that would hide this from the user(similarly to how we do for DenseElementsAttr). For the default behavior, many attributes already have a 'getValue' method which could be used directly. Otherwise, we could just return the derived attribute itself.

nicolasvasilache marked 6 inline comments as done.

Address review comments.

mlir/include/mlir/IR/Attributes.h
1472

getValue returns APInt so that does not work out of the box.
Returning an iterator over just the cast attribute to the proper type which simplifies the API.
Unpacking into the raw underlying type in the client where I have to materialize anyway.

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
737

yes, the other CL rebases on this.

Drop spurious forward declaration.

Unit tests: pass. 61305 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61305 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ftynse added inline comments.Jan 8 2020, 2:54 AM
mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
729

Why default 1 here, it's not obvious to me that getI64SubArray(attr) will drop the first element.

733

You need a range check, otherwise you risk reserving huge spaces when you drop more elements that you have due to unsigned arithmetic.

736

Would it->getValue() work ?

740

Please document this

803

What will happen to the operations we already inserted or deleted if we fail after modifying the IR? Have you tried it?

nicolasvasilache marked 6 inline comments as done.

Address review comments.

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp
736

Unfortunately no, it hits the following error:

llvm/include/llvm/ADT/iterator.h:169:34: error: taking the address of a temporary object of type 'mlir::IntegerAttr' [-Waddress-of-temporary]
  PointerT operator->() { return &static_cast<DerivedT *>(this)->operator*(); }

Unit tests: pass. 61319 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61319 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was not accepted when it landed; it landed in state Needs Review.Jan 9 2020, 12:16 AM
This revision was automatically updated to reflect the committed changes.
aganea added a subscriber: aganea.Jan 11 2020, 12:50 PM

Hi,
This patch fails to build with MSVC (here building with latest Visual Studio 2019 16.4.2).
Could you possibly take a look please? Thanks!

[801/3515] Building CXX object tools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\ConvertVectorToLLVM.cpp.obj
FAILED: tools/mlir/lib/Conversion/VectorToLLVM/CMakeFiles/MLIRVectorToLLVM.dir/ConvertVectorToLLVM.cpp.obj
C:\PROGRA~2\MIB055~1\2019\PROFES~1\VC\Tools\MSVC\1424~1.283\bin\HostX64\x64\cl.EXE  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\mlir\lib\Conversion\VectorToLLVM -ID:\llvm-project\mlir\lib\Conversion\VectorToLLVM -Iinclude -ID:\llvm-project\llvm\include -ID:\llvm-project\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zi /Zc:strictStrings /Oi /Zc:rvalueCast /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4345 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /MT /O2 /Ob2   -UNDEBUG  /EHs-c- /GR- -std:c++14 /showIncludes /Fotools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\ConvertVectorToLLVM.cpp.obj /Fdtools\mlir\lib\Conversion\VectorToLLVM\CMakeFiles\MLIRVectorToLLVM.dir\MLIRVectorToLLVM.pdb /FS -c D:\llvm-project\mlir\lib\Conversion\VectorToLLVM\ConvertVectorToLLVM.cpp
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): note: No constructor could take the source type, or constructor overload resolution was ambiguous
D:\llvm-project\mlir\lib\Conversion\VectorToLLVM\ConvertVectorToLLVM.cpp(117): note: see reference to function template instantiation 'llvm::iterator_range<mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>> mlir::ArrayAttr::getAsRange<mlir::IntegerAttr>(void)' being compiled
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): note: No constructor could take the source type, or constructor overload resolution was ambiguous
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(234): error C2672: 'llvm::make_range': no matching overloaded function found
D:\llvm-project\mlir\include\mlir/IR/Attributes.h(235): error C2780: 'llvm::iterator_range<IteratorT> llvm::make_range(T,T)': expects 2 arguments - 1 provided
D:\llvm-project\llvm\include\llvm/ADT/iterator_range.h(54): note: see declaration of 'llvm::make_range'
[874/2733] Building CXX object lib\Target\AArch64\CMakeFiles\LLVMAArch64CodeGen.dir\AArch64ISelDAGToDAG.cpp.obj
ninja: build stopped: subcommand failed.

Hi,
This patch fails to build with MSVC (here building with latest Visual Studio 2019 16.4.2).

If there is no fix readily available, I'll revert in the next few hours to unbreak.

Hi,
This patch fails to build with MSVC (here building with latest Visual Studio 2019 16.4.2).

If there is no fix readily available, I'll revert in the next few hours to unbreak.

Let's try to fix this. Reverting this will break some end-to-end tests that went in later.

my apologies, I lost track of this and do not have an easy repro atm, Aart is taking a shot at it.

flaub added a subscriber: flaub.Jan 16 2020, 2:22 AM
stella.stamenova removed a subscriber: stella.stamenova.

my apologies, I lost track of this and do not have an easy repro atm, Aart is taking a shot at it.

I have a repro if you need any information. The break looks the same as what was already shared (and we're also using latest VS):

##[error]mlir\include\mlir\IR\Attributes.h(234,0): Error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
   304>E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(234): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>' [E:\agent\_work\4\b\tools\mlir\lib\Conversion\VectorToLLVM\MLIRVectorToLLVM.vcxproj]
   394>ClCompile:
         CGStmt.cpp
   304>ClCompile:
         E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(234): note: No constructor could take the source type, or constructor overload resolution was ambiguous
         E:\agent\_work\4\s\mlir\lib\Conversion\VectorToLLVM\ConvertVectorToLLVM.cpp(119): note: see reference to function template instantiation 'llvm::iterator_range<mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>> mlir::ArrayAttr::getAsRange<mlir::IntegerAttr>(void)' being compiled
##[error]mlir\include\mlir\IR\Attributes.h(235,0): Error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>'
   304>E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(235): error C2440: '<function-style-cast>': cannot convert from 'mlir::ArrayAttr::iterator' to 'mlir::ArrayAttr::attr_value_iterator<mlir::IntegerAttr>' [E:\agent\_work\4\b\tools\mlir\lib\Conversion\VectorToLLVM\MLIRVectorToLLVM.vcxproj]
         E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(235): note: No constructor could take the source type, or constructor overload resolution was ambiguous
##[error]mlir\include\mlir\IR\Attributes.h(234,0): Error C2672: 'llvm::make_range': no matching overloaded function found
   304>E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(234): error C2672: 'llvm::make_range': no matching overloaded function found [E:\agent\_work\4\b\tools\mlir\lib\Conversion\VectorToLLVM\MLIRVectorToLLVM.vcxproj]
##[error]mlir\include\mlir\IR\Attributes.h(235,0): Error C2780: 'llvm::iterator_range<IteratorT> llvm::make_range(T,T)': expects 2 arguments - 1 provided
   304>E:\agent\_work\4\s\mlir\include\mlir/IR/Attributes.h(235): error C2780: 'llvm::iterator_range<IteratorT> llvm::make_range(T,T)': expects 2 arguments - 1 provided [E:\agent\_work\4\b\tools\mlir\lib\Conversion\VectorToLLVM\MLIRVectorToLLVM.vcxproj]
         E:\agent\_work\4\s\llvm\include\llvm/ADT/iterator_range.h(54): note: see declaration of 'llvm::make_range'