Page MenuHomePhabricator

[mlir][ASM] Refactor how attribute/type aliases are specified.
ClosedPublic

Authored by rriddle on Oct 13 2020, 5:02 PM.

Details

Summary

Previously they were separated into "instance" and "kind" aliases, and also required that the dialect know ahead of time all of the instances that would have a corresponding alias. This approach was very clunky and not ergonomic to interact with. The new approach is to provide the dialect with an instance of an attribute/type to provide an alias for, fully replacing the original split approach.

Diff Detail

Event Timeline

rriddle created this revision.Oct 13 2020, 5:02 PM
rriddle requested review of this revision.Oct 13 2020, 5:02 PM

Looks good overall

mlir/include/mlir/IR/OpImplementation.h
804–814

So if not overridden this just does nothing? "getAlias" which then writes to raw_ostream feels weird ... What is it getting?

mlir/lib/IR/AsmPrinter.cpp
238

Why not match with the gets above and have this and below be printAttribute? Below you have the type in the visit member too - Could we have these all consistent?

375

Why -1 ?

rriddle updated this revision to Diff 298481.Oct 15 2020, 3:03 PM
rriddle marked 3 inline comments as done.

update comments

Thanks for the review Jacques!

mlir/include/mlir/IR/OpImplementation.h
804–814

Switched to LogicalResult return.

mlir/lib/IR/AsmPrinter.cpp
375

This is what was used before to represent optionality(whew, this has been around for a while now), switched to Optional<int>.

Looks great!

I have two requests:

  • could we get tests that exercise sanization and name conflicts?
  • (separate commit) could we get a way to control alias printing (even something like -mlir-ignore-type-alias would work for me, looking up affine maps 1k lines above their use is a pain).
mlir/lib/IR/AsmPrinter.cpp
342–344

Are user-specified names allowed to start with an underscore? If so, what do we do in case sanization creates conflicts?

rriddle updated this revision to Diff 299820.Oct 21 2020, 4:14 PM
rriddle marked an inline comment as done.

Resolve comments and add tests

Thanks for the review Alex!

Looks great!

I have two requests:

  • could we get tests that exercise sanization and name conflicts?
  • (separate commit) could we get a way to control alias printing (even something like -mlir-ignore-type-alias would work for me, looking up affine maps 1k lines above their use is a pain).

If you use mlir-print-local-scope it should avoid printing aliases.

ftynse accepted this revision.Oct 29 2020, 7:19 AM

Thanks!

This revision is now accepted and ready to land.Oct 29 2020, 7:19 AM

I haven't had a chance to debug this yet, but we are seeing failures in the mlir tests on Windows Debug that appear to be caused by this change. The failures are:

MLIR :: Dialect/Affine/canonicalize.mlir
MLIR :: Dialect/Affine/simplify-affine-structures.mlir
MLIR :: Transforms/loop-invariant-code-motion.mlir

The failure in each of the tests is in initializeAliases. Here's an example error log:

         ********************
         FAIL: MLIR :: Transforms/loop-invariant-code-motion.mlir (67347 of 72158)
         ******************** TEST 'MLIR :: Transforms/loop-invariant-code-motion.mlir' FAILED ********************
         Script:
         --
         : 'RUN: at line 1';   e:\agent\_work\37\b\llvm\debug\bin\mlir-opt.exe E:\agent\_work\37\llvm-project\mlir\test\Transforms\loop-invariant-code-motion.mlir  -split-input-file -loop-invariant-code-motion | e:\agent\_work\37\b\llvm\debug\bin\filecheck.exe E:\agent\_work\37\llvm-project\mlir\test\Transforms\loop-invariant-code-motion.mlir
         --
         Exit Code: 1
         
         Command Output (stdout):
         --
         $ ":" "RUN: at line 1"
         note: command had no output on stdout or stderr
         $ "e:\agent\_work\37\b\llvm\debug\bin\mlir-opt.exe" "E:\agent\_work\37\llvm-project\mlir\test\Transforms\loop-invariant-code-motion.mlir" "-split-input-file" "-loop-invariant-code-motion"
         # command stderr:
         PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
         
         Stack dump:
		 
		  0.	Program arguments: e:\agent\_work\37\b\llvm\debug\bin\mlir-opt.exe E:\agent\_work\37\llvm-project\mlir\test\Transforms\loop-invariant-code-motion.mlir -split-input-file -loop-invariant-code-motion 
         
          #0 0x00007ff6737e151e std::_Vector_const_iterator<class std::_Vector_val<struct std::_Simple_types<class mlir::Attribute>>>::operator*(void) const c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\vector:51:0
         
          #1 0x00007ff6737e1818 std::_Vector_iterator<class std::_Vector_val<struct std::_Simple_types<class mlir::Attribute>>>::operator*(void) const c:\program files (x86)\microsoft visual studio\2017\community\vc\tools\msvc\14.16.27023\include\vector:259:0
         
          #2 0x00007ff6737f983f llvm::detail::result_pair<class std::vector<class mlir::Attribute, class std::allocator<class mlir::Attribute>> &>::value(void) e:\agent\_work\37\llvm-project\llvm\include\llvm\adt\stlextras.h:1825:0
         
          #3 0x00007ff6737bc0d7 initializeAliases<mlir::Attribute> e:\agent\_work\37\llvm-project\mlir\lib\ir\asmprinter.cpp:380:0
         
          #4 0x00007ff6737bb73f `anonymous namespace'::AliasState::AliasInitializer::initialize e:\agent\_work\37\llvm-project\mlir\lib\ir\asmprinter.cpp:407:0
         
          #5 0x00007ff6737bb25d `anonymous namespace'::AliasState::initialize e:\agent\_work\37\llvm-project\mlir\lib\ir\asmprinter.cpp:473:0
         
          #6 0x00007ff6737f57de mlir::detail::AsmStateImpl::initializeAliases(class mlir::Operation *) e:\agent\_work\37\llvm-project\mlir\lib\ir\asmprinter.cpp:878:0
         
          #7 0x00007ff6737baef9 mlir::ModuleOp::print(class llvm::raw_ostream &, class mlir::OpPrintingFlags) e:\agent\_work\37\llvm-project\mlir\lib\ir\asmprinter.cpp:2465:0
         
          #8 0x00007ff6734e3ecc performActions e:\agent\_work\37\llvm-project\mlir\lib\support\mliroptmain.cpp:74:0
		  
		  #9 0x00007ff6734e40e3 processBuffer e:\agent\_work\37\llvm-project\mlir\lib\support\mliroptmain.cpp:103:0
         
         #10 0x00007ff6734e6851 <lambda_cfae4fdf50565b622aa01f27d2c735fe>::operator() e:\agent\_work\37\llvm-project\mlir\lib\support\mliroptmain.cpp:133:0
         
         #11 0x00007ff6734e56c7 llvm::function_ref<(class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::raw_ostream &)>::callback_fn<class <lambda_cfae4fdf50565b622aa01f27d2c735fe>>(__int64, class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::raw_ostream &) e:\agent\_work\37\llvm-project\llvm\include\llvm\adt\stlextras.h:185:0
         
         #12 0x00007ff67375cbad llvm::function_ref<(class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::raw_ostream &)>::operator()(class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::raw_ostream &) const e:\agent\_work\37\llvm-project\llvm\include\llvm\adt\stlextras.h:209:0
         
         #13 0x00007ff67375c99d mlir::splitAndProcessBuffer(class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::function_ref<(class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class llvm::raw_ostream &)>, class llvm::raw_ostream &) e:\agent\_work\37\llvm-project\mlir\lib\support\toolutilities.cpp:42:0
         
         #14 0x00007ff6734e2e0c mlir::MlirOptMain(class llvm::raw_ostream &, class std::unique_ptr<class llvm::MemoryBuffer, struct std::default_delete<class llvm::MemoryBuffer>>, class mlir::PassPipelineCLParser const &, class mlir::DialectRegistry &, bool, bool, bool, bool, bool) e:\agent\_work\37\llvm-project\mlir\lib\support\mliroptmain.cpp:130:0
         
         #15 0x00007ff6734e3b9d mlir::MlirOptMain(int, char **, class llvm::StringRef, class mlir::DialectRegistry &, bool) e:\agent\_work\37\llvm-project\mlir\lib\support\mliroptmain.cpp:223:0
         
         #16 0x00007ff67115f230 main e:\agent\_work\37\llvm-project\mlir\tools\mlir-opt\mlir-opt.cpp:160:0
         
         #17 0x00007ff6739d86d4 invoke_main d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:79:0

I haven't had a chance to debug this yet, but we are seeing failures in the mlir tests on Windows Debug that appear to be caused by this change. The failures are:

If reverting the patch fixes it, please feel free to push a revert.

I haven't had a chance to debug this yet, but we are seeing failures in the mlir tests on Windows Debug that appear to be caused by this change. The failures are:

Sorry about that. Should be fixed by 235dfcf70abca65dba5d80f1a42d1485bab8980c.

I haven't had a chance to debug this yet, but we are seeing failures in the mlir tests on Windows Debug that appear to be caused by this change. The failures are:

Sorry about that. Should be fixed by 235dfcf70abca65dba5d80f1a42d1485bab8980c.

Thanks, all better now!