Page MenuHomePhabricator

[mlir][spirv] Plumbing target environment into type converter
ClosedPublic

Authored by antiagainst on Mar 16 2020, 11:51 AM.

Details

Summary

This commit unifies target environment queries into a new wrapper
class spirv::TargetEnv and shares across various places needing
the functionality. We still create multiple instances of TargetEnv
though given the parent components (type converters, passes,
conversion targets) have different lifetimes.

In the meantime, LowerABIAttributesPass is updated to take into
consideration the target environment, which requires udpates to
tests to provide that.

Depends On D75876

Diff Detail

Event Timeline

antiagainst created this revision.Mar 16 2020, 11:51 AM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
mravishankar requested changes to this revision.Mar 16 2020, 12:22 PM
mravishankar added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
77

Added a comment on the previous CL in this stack, but just mirroring here. I think its would be good to have a get method which will set the default target env. Its seems more ergonomic to me.

mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
54

These might cause subtle issues later on. Can we make this a SetVector?

This revision now requires changes to proceed.Mar 16 2020, 12:22 PM
antiagainst marked 4 inline comments as done.Mar 17 2020, 6:58 AM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
77

Replied in the previous CL.

mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
54

But what we want here is just a set. We don't care about the order. What problems do you think will have with SmallSet?

mravishankar accepted this revision.Mar 17 2020, 2:53 PM
mravishankar added inline comments.
mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
54

For now this is not iterated over. But there is no gaurantee later on and is easy to miss. Dont see too much advantage of using SmallSet vs SetVector, and just prefer to use the latter.

This revision is now accepted and ready to land.Mar 17 2020, 2:53 PM
antiagainst marked 4 inline comments as done.Mar 18 2020, 6:20 AM
antiagainst added inline comments.
mlir/include/mlir/Dialect/SPIRV/TargetAndABI.h
54

Yeah I think we can re-evaluate this later when we want to loop over the set. Likely we will not have that need; I'd assume querying whether some extensions/capabilities are allowed is all what we need for using the target environment. But you never know. :)

This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.