This is an archive of the discontinued LLVM Phabricator instance.

[SPIR-V] Support TargetExtType for SPIR-V builtin types
ClosedPublic

Authored by mpaszkowski on Feb 21 2023, 8:02 AM.

Details

Summary

This patch adds support for TargetExtType/target(...) representing SPIR-V builtin types. After D135202, target(...) is the preferred way for representing SPIR-V builtin types in LLVM IR and the only working in the opaque pointer mode.

In order to maintain compatibility with LLVM IR generated by older versions of Clang and LLVM/SPIR-V Translator, pointers-to-opaque-structs denoting SPIR-V/OpenCL builtin types will be translated to equivalent SPIR-V target extension types. This translation is only available in the typed pointer mode (-opaque-pointers=0).

The relevant LIT tests with SPIR-V builtins were converted to use the new target(...) notation.

Diff Detail

Event Timeline

mpaszkowski created this revision.Feb 21 2023, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 8:02 AM
mpaszkowski requested review of this revision.Feb 21 2023, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 8:02 AM

There still remains a large number of LIT tests with pointers-to-opaque-structs representing OpenCL builtin types. I would suggest to do either one of these things in the next patch:

  1. Leave these tests in the current form (with -opaque-pointers=0)
  2. Convert the tests to use SPIR-V builtin types instead of OpenCL builtin types
  3. Convert the LIT tests to OpenCL C end-to-end tests (using Clang and llc)

I think converting most of the tests to be based on SPIR-V builtin types (2) and only keeping a handful of tests using pointers-to-opaque-struct (1) to demonstrate compatibility with old IR makes most sense.

Fixed formatting

I think converting most of the tests to be based on SPIR-V builtin types (2) and only keeping a handful of tests using pointers-to-opaque-struct (1) to demonstrate compatibility with old IR makes most sense.

I agree.

Thanks for the patch. Overall, it looks good to me. Minor suggestions are noted below.

llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
1974

Maybe use the same style as in previous comment (line 1961):

// Pointers-to-opaque-structs representing OpenCL types are first translated
// to equivalent SPIR-V types. OpenCL builtin type names should have the
// following format: e.g. %opencl.event_t
1975–1976

It could be assert().

2095

Double space in "records or".

mpaszkowski marked 3 inline comments as done.

Thanks @iliya-diyachkov! Fixed!

iliya-diyachkov accepted this revision.Feb 27 2023, 9:21 AM
This revision is now accepted and ready to land.Feb 27 2023, 9:21 AM
jcranmer-intel added inline comments.Mar 1 2023, 10:26 AM
llvm/lib/IR/Type.cpp
865

Why do you need to support both spirv.* and opencl.* opaque types?

llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
1977

Nit: "Parameterized"

1987

Nit: HasTypeParameter.

2009–2010

Nit: getNonParameterizedType.

mpaszkowski added inline comments.Mar 1 2023, 11:04 AM
llvm/lib/IR/Type.cpp
865

@jcranmer-intel Hi Joshua, Thanks for catching this! I think I forgot to remove it after experimenting with translating OpenCL types to SPIR-V types in the previous version of the patch. We don't need this anymore. I will remove this and fix the typos in a new commit when I get back home.