Page MenuHomePhabricator

[MLIR][SPIRV] Support two memory access attributes in OpCopyMemory.
ClosedPublic

Authored by ergawy on Jul 6 2020, 9:51 AM.

Details

Summary

This commit augments spv.CopyMemory's implementation to support 2 memory
access operands. Hence, more closely following the spec. The following
changes are introduces:

  • Customize logic for spv.CopyMemory serialization and deserialization.
  • Add 2 additional attributes for source memory access operand.

Diff Detail

Event Timeline

ergawy created this revision.Jul 6 2020, 9:51 AM
ergawy edited the summary of this revision. (Show Details)Jul 6 2020, 9:52 AM
ergawy updated this revision to Diff 275750.Jul 6 2020, 9:56 AM

Get rid of decltype inside template.

Harbormaster completed remote builds in B63040: Diff 275746.
ergawy updated this revision to Diff 275918.Jul 6 2020, 11:41 PM

Handle some small review comments:

  • Remove braces from signle-statement if.
  • Remove username from TODO.
bondhugula added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
170

This function is completely missing doc comments. There should typically be a syntax example for parse routines.

bondhugula requested changes to this revision.Jul 8 2020, 1:11 AM
This revision now requires changes to proceed.Jul 8 2020, 1:11 AM
ergawy updated this revision to Diff 276446.Jul 8 2020, 8:32 AM

Add more docs.

ergawy marked an inline comment as done.Jul 8 2020, 8:32 AM

Unfortunately, this still produces a cryptic error on VS2017:

##[error]mlir\lib\Dialect\SPIRV\SPIRVOps.cpp(2865,0): Error C1001: An internal error has occurred in the compiler.
   597>E:\agent\_work\4\s\mlir\lib\Dialect\SPIRV\SPIRVOps.cpp(2865): fatal error C1001: An internal error has occurred in the compiler. [E:\agent\_work\4\b\llvm\tools\mlir\lib\Dialect\SPIRV\obj.MLIRSPIRV.vcxproj]
         (compiler file 'msc1.cpp', line 1518)
          To work around this problem, try simplifying or changing the program near the locations listed above.
ergawy added a comment.EditedJul 8 2020, 11:48 PM

Unfortunately, this still produces a cryptic error on VS2017:

##[error]mlir\lib\Dialect\SPIRV\SPIRVOps.cpp(2865,0): Error C1001: An internal error has occurred in the compiler.
   597>E:\agent\_work\4\s\mlir\lib\Dialect\SPIRV\SPIRVOps.cpp(2865): fatal error C1001: An internal error has occurred in the compiler. [E:\agent\_work\4\b\llvm\tools\mlir\lib\Dialect\SPIRV\obj.MLIRSPIRV.vcxproj]
         (compiler file 'msc1.cpp', line 1518)
          To work around this problem, try simplifying or changing the program near the locations listed above.

@NathanielMcVicar thanks for pointing that out before merging. I was hoping that removing decltype from inside the template brackets would help solve the issue. I don't have a Windows machine so it's difficult to debug the problem. I will try to setup some virtual machine if possible. However, it would be great if you can give it a try and find out which part is causing the problem. (No need thanks, I setup a virtual environment)

ergawy updated this revision to Diff 276747.Jul 9 2020, 8:36 AM

Split template functions into twins.

ergawy marked an inline comment as done.Jul 9 2020, 8:40 AM
ergawy added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
197

Unfortunately, no matter how much I simplify the template usage (remove namespaces, extract calls into variables, and such things) VS2017 still failed with an internal error. Therefore, I had to split these sets of functions into pairs.

(No need thanks, I setup a virtual environment)

You don't need anything from me then? The effort to test additional patches is pretty minimal, but if you're all good that's even better :). Thanks for dealing with this, I know it's a pain to debug.

antiagainst accepted this revision.Jul 9 2020, 4:08 PM

Thanks @ergawy for bearing with all the pain to make this work. I can understand how much additional effort it is to set up a VM for iterating on this. And really appreciate your contribution here.

@NathanielMcVicar it would be really good to have a way exposed to patch authors to try out a patch onto LLVM's post-commit CI. :)

This revision was not accepted when it landed; it landed in state Needs Review.Jul 9 2020, 5:24 PM
This revision was automatically updated to reflect the committed changes.

@NathanielMcVicar Oh BTW, please certainly feel free to ignore my previous suggestion if it didn't make sense. I don't manage the CIs myself so I can be way off there. And really appreciate your help in reporting the issue and try out patches!