Page MenuHomePhabricator

[mir][Python][linalg] Support OpDSL extensions in C++.

Authored by gysit on May 7 2021, 8:14 AM.



The patch extends the yaml code generation to support the following new OpDSL constructs:

  • captures
  • constants
  • iteration index accesses
  • predefined types

These changes have been introduced by revision

Diff Detail

Event Timeline

gysit created this revision.May 7 2021, 8:14 AM
gysit requested review of this revision.May 7 2021, 8:14 AM
gysit added a comment.May 7 2021, 8:33 AM

I decided to add the following tests:

  • extended the existing OpDSL tests and added and to the python/dialect/linalg/opdsl folder
  • added a test to verify basic yaml codgen properties in test-linalg-ods-yaml-gen.yaml in the folder test/mlir-linalg-ods-gen
  • added a test for the fill_rng_2d operation in generalize-named-polymorphic-ops.mlir in the folder test/Dialect/linalg
  • added mini integration test in in the python/dialect/linalg/ folder
stellaraccident accepted this revision.May 9 2021, 11:57 PM

This has come along nicely since we looked at it together - thank you for spending the time on adding an additional layer of testing. Code lgtm - I'll leave to Nicolas wrt details of Linalg parse/print/etc.


In Python, one generally just says if self.ordered_capture_args and let the default empty-container-to-False rule apply.


Can be done in a follow-up, but I feel like this test is firmly in integration test territory and should probably be moved to the integration test directory.

This revision is now accepted and ready to land.May 9 2021, 11:57 PM
gysit updated this revision to Diff 344363.May 11 2021, 5:10 AM
gysit marked an inline comment as done.

Address review comment and rebase.

gysit added inline comments.May 11 2021, 5:19 AM

The dash (-) is used here to write the result to the standard output instead of writing to a file (hope this trick works on all platforms but mlir-linalg-ods-gen uses (-) as default file name so should be fine)


yes lets move this to a follow-up iteration

This revision does a little too much for my taste.
I don't want to burden you with splitting this in 4 independent CLs one per bullet point but in the future please heavily bias towards small independent CLs that are easy to just click.

Still, I think at least the captures should be separated out because it does increase the semantic power of linalg ops irrespective of opdsl.
It feels like there should be some roundtrip / invalid test, extensions to the structuredopinterface etc.
Or maybe I missing something that makes this simpler than what my gut feeling tells me?

In any case let's iterate Monday on VC.



nit: NL


nit: NL

15 ↗(On Diff #344363)

nit: NL

20 ↗(On Diff #344363)

I am unclear why this test is necessary, what does it cover that was not covered before ?


nit: NL

gysit updated this revision to Diff 346143.Tue, May 18, 5:00 AM

Address reviewer comment:

  • Remove capture support
  • Formatting
gysit marked 4 inline comments as done.Tue, May 18, 5:06 AM
gysit updated this revision to Diff 346153.Tue, May 18, 5:52 AM

Activate and fix the yaml generation test.

gysit updated this revision to Diff 346159.Tue, May 18, 6:27 AM

Fix lit configuration.

nicolasvasilache accepted this revision.Wed, May 19, 4:37 AM
This revision was landed with ongoing or failed builds.Wed, May 19, 6:37 AM
This revision was automatically updated to reflect the committed changes.

I believe this change causes the assert on the attribute SmallVector (for size >256) to hit when the size isn't specified on Windows debug builds.

[4242/5334] Building CXX object tools\mlir\tools\mlir-linalg-ods-gen\CMakeFiles\mlir-linalg-ods-yaml-gen.dir\mlir-linalg-ods-yaml-gen.cpp.obj
FAILED: tools/mlir/tools/mlir-linalg-ods-gen/CMakeFiles/mlir-linalg-ods-yaml-gen.dir/mlir-linalg-ods-yaml-gen.cpp.obj 
C:\PROGRA~2\MICROS~3\2019\COMMUN~1\VC\Tools\MSVC\1428~1.299\bin\Hostx64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=0 -DMLIR_ROCM_CONVERSIONS_ENABLED=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\tools\mlir-linalg-ods-gen -IC:\vsts-agent\_work\13\s\mlir\tools\mlir-linalg-ods-gen -Iinclude -IC:\vsts-agent\_work\13\s\llvm\include -IC:\vsts-agent\_work\13\s\mlir\include -Itools\mlir\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -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 /MDd /Zi /Ob0 /Od /RTC1  /EHs-c- /GR- /EHs-c- /GR- -std:c++14 /showIncludes /Fotools\mlir\tools\mlir-linalg-ods-gen\CMakeFiles\mlir-linalg-ods-yaml-gen.dir\mlir-linalg-ods-yaml-gen.cpp.obj /Fdtools\mlir\tools\mlir-linalg-ods-gen\CMakeFiles\mlir-linalg-ods-yaml-gen.dir\ /FS -c C:\vsts-agent\_work\13\s\mlir\tools\mlir-linalg-ods-gen\mlir-linalg-ods-yaml-gen.cpp
C:\vsts-agent\_work\13\s\llvm\include\llvm/ADT/SmallVector.h(1135): error C2338: You are trying to use a default number of inlined elements for `SmallVector<T>` but `sizeof(T)` is really big! Please use an explicit number of inlined elements with `SmallVector<T, N>` to make sure you really want that much inline storage.
C:\vsts-agent\_work\13\s\mlir\tools\mlir-linalg-ods-gen\mlir-linalg-ods-yaml-gen.cpp(120): note: see reference to class template instantiation 'llvm::CalculateSmallVectorDefaultInlinedElements<T>' being compiled
C:\vsts-agent\_work\13\s\mlir\tools\mlir-linalg-ods-gen\mlir-linalg-ods-yaml-gen.cpp(120): error C2976: 'llvm::SmallVector': too few template arguments
C:\vsts-agent\_work\13\s\llvm\include\llvm/ADT/SmallVector.h(1168): note: see declaration of 'llvm::SmallVector'

@NathanielMcVicar Thanks for reporting this. I hope fixes your issue?