This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Properly support SPIR-V conversion target
ClosedPublic

Authored by antiagainst on Jan 6 2020, 5:37 AM.

Details

Summary

This commit defines a new SPIR-V dialect attribute for specifying
a SPIR-V target environment. It is a dictionary attribute containing
the SPIR-V version, supported extension list, and allowed capability
list. A SPIRVConversionTarget subclass is created to take in the
target environment and sets proper dynmaically legal ops by querying
the op availability interface of SPIR-V ops to make sure they are
available in the specified target environment. All existing conversions
targeting SPIR-V is changed to use this SPIRVConversionTarget. It
probes whether the input IR has a spv.target_env attribute,
otherwise, it uses the default target environment: SPIR-V 1.0 with
Shader capability and no extra extensions.

Diff Detail

Event Timeline

antiagainst created this revision.Jan 6 2020, 5:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 5:37 AM

Remove unrelated commits

Unit tests: pass. 61250 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Unit tests: pass. 61250 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

antiagainst edited the summary of this revision. (Show Details)Jan 9 2020, 10:27 AM

Hey @rriddle, Mahesh will be OOO for a while. Could you PTAL this CL? Thanks! :)

What are the constraints here? Who is populating the target attribute on the module? How is this specified from a hypothetical front-end? I ask mainly because we are intending to have a general solution for "target" availability, that should(ideally) be a more general form of some the things introduced here.

mlir/include/mlir/Dialect/SPIRV/SPIRVLowering.h
73

Use /// instead of trailing //.

mlir/include/mlir/Dialect/SPIRV/TargetAndABI.td
32–36

nit: space before the :

58

nit: Remove the extra newline

antiagainst marked 3 inline comments as done.
Address comments

Unit tests: pass. 61585 tests passed, 0 failed and 777 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

What are the constraints here?

From the compiler's point of view, a SPIR-V target environment is mainly specified via three components: version, extensions, and capabilities. Constraints on them are expressed via verifications on the TargetEnvAttr; there are tests for them in this commit. A TargetEnvAttr should be attached to the outermost IR unit going through the SPIR-V compilation path.

Who is populating the target attribute on the module?
How is this specified from a hypothetical front-end?

It is expected to be attached by whoever invoking the SPIR-V compilation flow. Right now I only enabled specifying it via an attribute on the input IR unit. It can also be exposed via CLI options like --mlir-spirv-target-version, --mlir-spirv-allowed-extensions, --mlir-spirv-allowed-capabilities or whatever.

I ask mainly because we are intending to have a general solution for "target" availability, that should(ideally) be a more general form of some the things introduced here.

That's certainly very useful and long awaited. I'm happy to adopt it when available; also feel free to take whatever useful here.

benvanik accepted this revision.Jan 14 2020, 1:42 PM
benvanik added a subscriber: benvanik.
benvanik added inline comments.
mlir/test/Dialect/SPIRV/target-env.mlir
22

I think I get why the capability needs to be an int (as you wouldn't know the encoding of unknown capabilities in the compiler otherwise), but to improve the readability of the MLIR what about adding comments on the spv.target_env printer like capabilities = [6, 7]} // ["Foo", "Bar"] for known ones? Would help for those of us who may want to ensure the IR is referencing the right caps but don't have the IDs memorized :)

This revision is now accepted and ready to land.Jan 14 2020, 1:42 PM
antiagainst marked an inline comment as done.Jan 14 2020, 3:20 PM

Thanks for the review, Ben!!

mlir/test/Dialect/SPIRV/target-env.mlir
22

Using numbers also helps when doing capability probing/comparison in conversion target. (We cannot do the same for extensions because extensions do not have a spec-defined number.) I don't think we can control how an attr is printed, unless to create a dialect-specific attribute. That is an improvement that can be deferred; I'll look into it later. :)

This revision was automatically updated to reflect the committed changes.