Page MenuHomePhabricator

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

Authored by ergawy on Jun 28 2020, 1:54 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.Jun 28 2020, 1:54 AM
ergawy updated this revision to Diff 273912.Jun 28 2020, 2:21 AM
  • Add TODO to verify the case of 2 two memroy access operands.
ergawy updated this revision to Diff 273932.Jun 28 2020, 6:58 AM
  • Better style to test verification failure.
ergawy updated this revision to Diff 273933.Jun 28 2020, 7:04 AM
  • Update CopyMemory production rule in docs.
Harbormaster completed remote builds in B62046: Diff 273932.
antiagainst accepted this revision.Jun 29 2020, 6:05 PM

Nice! Just some minor issues.

mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
194

I think we typically put mandated parameters first?

268

Same here.

mlir/test/Dialect/SPIRV/ops.mlir
1270

We can omit the -> () part actually. (Sorry existing test cases have that due to historical reasons.)

This revision is now accepted and ready to land.Jun 29 2020, 6:05 PM
ergawy updated this revision to Diff 274443.Jun 30 2020, 6:13 AM
  • Add TODO to verify the case of 2 two memroy access operands.
  • Better style to test verification failure.
  • Update CopyMemory production rule in docs.
  • Handle review comments:
ergawy marked 3 inline comments as done.Jun 30 2020, 6:16 AM

@antiagainst Thanks for the review. Handled the issues.

It seems that I still don't have commit access. Just to be sure that I didn't do anything wrong. Here what I tried:
1 - I squashed all the commits in my local dev branch into one. Also in the commit messages cleaned up the unnecessary Arcanist tags except for Differential Revision: ...
2 - Rebased on top of master.
3 - Changed the active branch to master.
4 - Cherry picked the squashed commit from step 1.
5 - Tried to: git push.

What I get is:

remote: Permission to llvm/llvm-project.git denied to KareemErgawy.
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': The requested URL returned error: 403

Please update commit title: 'Support 2' -> 'Support two'

ergawy updated this revision to Diff 275119.Jul 2 2020, 7:35 AM

Update commit title.

ergawy retitled this revision from [MLIR][SPIRV] Support 2 memory access attributes in OpCopyMemory. to [MLIR][SPIRV] Support two memory access attributes in OpCopyMemory..Jul 2 2020, 7:36 AM

It seems that I still don't have commit access. Just to be sure that I didn't do anything wrong. Here what I tried:
1 - I squashed all the commits in my local dev branch into one. Also in the commit messages cleaned up the unnecessary Arcanist tags except for Differential Revision: ...
2 - Rebased on top of master.
3 - Changed the active branch to master.
4 - Cherry picked the squashed commit from step 1.
5 - Tried to: git push.

What I get is:

remote: Permission to llvm/llvm-project.git denied to KareemErgawy.
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': The requested URL returned error: 403

Yup. There is a procedure to request direct write access to the mono repo: https://llvm.org/docs/DeveloperPolicy.html#new-contributors. Typically one needs to accumulate a few commits first before asking. I can help landing CLs before that. :)

This revision was automatically updated to reflect the committed changes.

Hey @ergawy, I reverted this in https://reviews.llvm.org/D83075. This causes a VS2017 MSVC internal compiler bug: http://lab.llvm.org:8011/builders/mlir-windows/builds/4147/steps/build-unified-tree/logs/stdio. I don't know the exact reason; maybe due to the templating or decltype? Might want to cherry-pick the original patch and then modify it to avoid using those "advanced" features to give it a try.

I can help to take a look after coming back from the long weekend. Sorry about the back and forth!

Hey @ergawy, I reverted this in https://reviews.llvm.org/D83075. This causes a VS2017 MSVC internal compiler bug: http://lab.llvm.org:8011/builders/mlir-windows/builds/4147/steps/build-unified-tree/logs/stdio. I don't know the exact reason; maybe due to the templating or decltype? Might want to cherry-pick the original patch and then modify it to avoid using those "advanced" features to give it a try.

I can help to take a look after coming back from the long weekend. Sorry about the back and forth!

That's surprising! And here I am thinking I don't know enough "advanced" C++, turns out I know enough to break a compiler :P.

No worries, enjoy your weekend. Will try without the potentially problematic features.

rriddle added inline comments.Jul 6 2020, 6:39 PM
mlir/lib/Dialect/SPIRV/SPIRVOps.cpp
206

nit: Drop trivial braces.

2902

Same here and below.

2937

nit: Please do not put user names in TODO comments.

ergawy marked 3 inline comments as done.EditedJul 6 2020, 11:43 PM

@rriddle Thanks for your comments. I handled them but in this review: https://reviews.llvm.org/D83241. This one had to be reverted due to some internal error in MSVC.

@rriddle Thanks for your comments. I handled them but in this review: https://reviews.llvm.org/D83241. This one had to be reverted due to some internal error in MSVC.

No worries, thanks for updating the other review.