Page MenuHomePhabricator

[mlir][spirv] Let SPIRVConversionTarget consider type availability
ClosedPublic

Authored by antiagainst on Mar 9 2020, 2:10 PM.

Details

Summary

Previously we only consider the version/extension/capability requirement
on the op itself. This commit updates SPIRVConversionTarget to also
take into consideration the values' types when deciding op legality.

Depends On D75875

Diff Detail

Event Timeline

antiagainst created this revision.Mar 9 2020, 2:10 PM
Herald added a reviewer: herhut. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
herhut resigned from this revision.Mar 12 2020, 4:30 AM
mravishankar requested changes to this revision.Mar 16 2020, 12:12 PM
mravishankar added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
414

Could we add a method that sets some targetEnv by default (the one that are used in the tests here for example).

This revision now requires changes to proceed.Mar 16 2020, 12:12 PM
mravishankar added inline comments.Mar 16 2020, 12:15 PM
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
414

Just to add to this. Then we can remove the "lookupTargetEnvOrDefault" method right?

antiagainst marked 3 inline comments as done.Mar 17 2020, 6:57 AM
antiagainst added inline comments.
mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp
414

I'd like to keep the responsibility separate. We already have three helper functions for getting the target env: getDefaultTargetEnv, lookupTargetEnv, and lookupTargetEnvOrDefault. This class should be focusing on conversion target. If one want to just use the default target env, one can use getDefaultTargetEnv to create a SPIRVConversionTarget.

mravishankar accepted this revision.Mar 17 2020, 2:50 PM
This revision is now accepted and ready to land.Mar 17 2020, 2:50 PM
This revision was automatically updated to reflect the committed changes.
antiagainst marked an inline comment as done.