This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add support for OpTypeSampledImage
ClosedPublic

Authored by Weiwei-2021 on Feb 5 2021, 11:49 AM.

Details

Summary

Support OpTypeSampledImage in SPIRV Dialect.

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

Diff Detail

Event Timeline

Weiwei-2021 created this revision.Feb 5 2021, 11:49 AM
Weiwei-2021 requested review of this revision.Feb 5 2021, 11:49 AM
antiagainst requested changes to this revision.Feb 6 2021, 6:47 AM

Awesome, looks nice. Could you also add tests to test/Dialect/SPIRV/IR/types.mlir? That one is for testing type/op validaty. test/Target/SPIRV/ is just for serialization/deserialization roundtrip.

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVBase.td
3320–3321

Hmm, the text wrapping seems a bit off. Did you use the script under utils/spirv to add it here? You can

cd utils/spirv
./define_opcodes.sh OpTypeSampledImage

It will automatically pull in the opcode from the spec and update the enum list here, including reflowing .

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
244

Nit: could you use a few empty lines to separate the method declarations to make it more clear like the above types?

249

Can you add a verifyConstructionInvariants to check that the enclosed type is indeed an image type? (Aslo add tests for it to check error cases.) You can use MatrixType::verifyConstructionInvariants as a reference.

mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
732

We can let this method to return spirv::ImageType directly to avoid requiring all callers to cast?

This revision now requires changes to proceed.Feb 6 2021, 6:47 AM
bondhugula added inline comments.
mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp
759

Nit: I notice that you use camel case at some places "spv.globalVariable" while all lower here. Consider "sampledImage" or snake case "sampled_image" for consistency and readability.

Weiwei-2021 marked 4 inline comments as done.

I am not familiar with adding test for mlir, please let me know whether the way I add API it right or wrong. Thanks!

Weiwei-2021 marked an inline comment as not done.Feb 8 2021, 5:47 PM
Weiwei-2021 added inline comments.
mlir/include/mlir/Dialect/SPIRV/IR/SPIRVBase.td
3320–3321

Thank you for telling! I don't know that there is a script for it. I manually added it....

mlir/include/mlir/Dialect/SPIRV/IR/SPIRVTypes.h
249

I added test and some API for it. Please let me know whether I did in a correct way!

mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp
759

Thanks for pointing it out!

mlir/lib/Dialect/SPIRV/IR/SPIRVTypes.cpp
732

I tried to return spirv::ImageType but triggered an error. I check MatrixTyps, it seems that it use the same approach to return its type.

ergawy added inline comments.Feb 9 2021, 3:25 AM
mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp
556

nit: Can you replace parseVal with parsedType? I think it makes the code more readable since the name related more closely to what it represents.

Also, as long as the var type is not that complicated/difficult to deduce (here it's just Type), then it would be better to explicitly state its type instead of auto.

556

Any reason why not inline parseAndVerifySampledImageType here and get rid of it above?

Generally LGTM. Could you also update SPIR-V.md regarding the new changes? Thanks!

mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp
759

Actually, we should be using all lower cases here to be consistent with other types. Could you change it to sampled_image?

Regarding spv.globalVariable, it should actually be spv.GlobalVariable to be consistent with other ops. That is something to be cleaned up but not yet done. Sorry for the confusion!

antiagainst requested changes to this revision.Feb 9 2021, 6:46 AM
This revision now requires changes to proceed.Feb 9 2021, 6:46 AM
Weiwei-2021 marked 2 inline comments as done and an inline comment as not done.
Weiwei-2021 added inline comments.
mlir/lib/Dialect/SPIRV/IR/SPIRVDialect.cpp
556

I changed the variable and auto. Thanks. About the parseAndVerify function, I refer to Matrix Type to write this part. It reads like that we want to put the verify part is in a separate function, so I follow it.

759

good to know. Thanks for correction.

antiagainst accepted this revision.Feb 9 2021, 10:42 AM
This revision is now accepted and ready to land.Feb 9 2021, 10:42 AM

Thanks for contributing and also thanks for the patience to tolerate multiple runs of my nitpicking. :) The first few patches typically see more of that as a way to establish coding conventions. I expect it to happen less in the future.

This revision was automatically updated to reflect the committed changes.
Weiwei-2021 marked 3 inline comments as done.Feb 9 2021, 1:03 PM

Thanks for contributing and also thanks for the patience to tolerate multiple runs of my nitpicking. :) The first few patches typically see more of that as a way to establish coding conventions. I expect it to happen less in the future.

Thank you very much for your review and explanation all the time. Like I mentioned at the beginning, I am a newbie in both MLIR and contrinbuing code to an open source community. I do need those advices! Appreciate for you time and help!