Page MenuHomePhabricator

[mlir][spirv] Add support for OpImageType
ClosedPublic

Authored by Weiwei-2021 on Jan 27 2021, 6:32 PM.

Details

Summary

Support OpImageType in SPIRV Dialect.

This change doesn't support operand AccessQualifier since it is optinal and only enables under Kernel capability.

co-authored-by: Alan Liu <alanliu.yf@gmail.com>

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Jan 27 2021, 6:32 PM
Weiwei-2021 requested review of this revision.Jan 27 2021, 6:32 PM
ergawy added a subscriber: ergawy.Jan 28 2021, 2:54 AM
ergawy added inline comments.
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1012

small nit: I think the following if condition is more appropriate for this check. We can remove this line.

(Hope you don't mind me passing by :) ).

Weiwei-2021 added inline comments.Jan 28 2021, 8:09 AM
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1012

:) Thank you for reviewing! I will remove this assertion and upload again (after I figure out how to upload a revision. This is my first time using this system - still learning. :p)

ergawy added inline comments.Jan 28 2021, 8:31 AM
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1012

One way to do that is:

  • Do whatever changes on your working copy (i.e. remove the assert in this case).
  • git add <file> and git commit --amend.
  • arc diff origin/main, arcanist should detect that you want to update this revision (rather than open a new one).

So, basically, updating a revision is the same flow as opening a new one with the exception of amending to the already existing commit (most of the time at least). Hope that helps.

Remove a redundant assertion.

Weiwei-2021 added inline comments.Jan 28 2021, 10:10 AM
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1012

Thank you for the explanation! I updated it.

antiagainst requested changes to this revision.Jan 28 2021, 1:45 PM

Looks awesome! I just have a few nits here. Thanks for pushing on it!

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1013

Nit: It's not that OpTypeImage must have eight operands (as per the spec). I think it's more that "OpTypeImage with non-eight operands are not supported yet". :)

1021

We should check the Optional and return errors. Directly getValue can crash given bad SPIR-V input.

1027

This should be checked in the type itself (like using verifyConstructionInvariants). In general the deserializer is a tool that reads in SPIR-V blob and generates SPIR-V IR. For the deserializer's concern it should only care about the input binary's validity: whether the input blob is encoded properly. Checks for semantic rules should be on the type/op itself. This separates concerns. Also generally in IR form we can have better error report (like getting the exact IR dumped with line #), where we cannot get it here. :)

mlir/test/Target/SPIRV/image.mlir
4

I feel we don't need all these combinations: if one enumerant works, others in the same enum will work too. So a few cases to vary as much as possible would be good enough. :)

This revision now requires changes to proceed.Jan 28 2021, 1:45 PM
Weiwei-2021 set the repository for this revision to rG LLVM Github Monorepo.

Apply some changes based on comments.

Weiwei-2021 marked 4 inline comments as done.Jan 29 2021, 9:56 AM
Weiwei-2021 added inline comments.
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1013

Good point. Thanks!

1021

Thanks for pointing this out! Curently, I checked each operand separately. Hope this is fine.

1027

Thanks for the explanation. I decide to delete this part from this function. Maybe we need to consider this in some other stage, but like you said, that is a separated concern. :)

mlir/test/Target/SPIRV/image.mlir
4

Only keep three combinations here. Notice that the var number is unordered. I will fixed it later.

Weiwei-2021 marked 4 inline comments as done.

Fix a numbering issue in test.

mlir/test/Target/SPIRV/image.mlir
4

Only keep three combinations here. Notice that the var number is unordered. I will fixed it later.

fix the numbering now.

antiagainst requested changes to this revision.Jan 30 2021, 3:43 AM

Awesome, just one final nit from me.

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1023

It isn't missing. It's "unknown Dim for OpTypeImage: "

Similarly for others. :)

This revision now requires changes to proceed.Jan 30 2021, 3:43 AM

Correct one work for error handling

Weiwei-2021 marked an inline comment as done.Feb 1 2021, 8:04 AM
Weiwei-2021 added inline comments.
mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp
1023

Good catch! Thank you.

antiagainst accepted this revision.Feb 1 2021, 11:30 AM

Awesome, thanks!

This revision is now accepted and ready to land.Feb 1 2021, 11:30 AM

Oops, sorry the patch (https://github.com/llvm/llvm-project/commit/21f1462106b9ee1e646bf409c85528828320b34e) gotten landed has incorrect author. So I reverted it. Could you let me know the Author field I should use for submitting the patch?

Weiwei-2021 marked an inline comment as done.Feb 1 2021, 12:12 PM

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <weiwei.li1@huawei.com>".

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <weiwei.li1@huawei.com>".

Awesome, thanks! Fixed and relanded as https://github.com/llvm/llvm-project/commit/35f746c17fd0315b235050c0a350fc521f944f20.

@antiagainst Thank you for start mering this commit! The author filed is "Weiwei Li <weiwei.li1@huawei.com>".

Awesome, thanks! Fixed and relanded as https://github.com/llvm/llvm-project/commit/35f746c17fd0315b235050c0a350fc521f944f20.

Thank you for all your help on it! I will start contributing more our work on MLIR.