This is an archive of the discontinued LLVM Phabricator instance.

libclc: Add Mesa/SPIR-V target
ClosedPublic

Authored by daniels on Apr 6 2020, 2:26 PM.

Details

Summary

Add targets to emit SPIR-V targeted to Mesa's OpenCL support, using
SPIR-V 1.1.

Substantially based on Dave Airlie's earlier work.

libclc: spirv: remove step/smoothstep apis not defined for SPIR-V

libclc: disable inlines for SPIR-V builds

Diff Detail

Event Timeline

daniels created this revision.Apr 6 2020, 2:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 2:27 PM

Just as an FYI, this is what we are actually using together with a few WIP branches in Mesa. Of note is that we run the LLVM StructurizeCFG pass before generating SPIR-V, else vtn chokes on it, even with the WIP Mesa structuriser.

@jvesely @tstellar I think this should be good for you to both review and land. We've been using this in multiple drivers within Mesa (the OpenCL-on-DirectX backend, software llvmpipe, Nouveau), and it seems to work for us all so far.

tstellar added inline comments.May 22 2020, 8:15 AM
libclc/spirv-mesa3d-dxil/lib/subnormal_config.cl
1 ↗(On Diff #255494)

What's going on with this file? Is subnormal_config.cl a new file?

daniels added inline comments.May 22 2020, 9:39 AM
libclc/spirv-mesa3d-dxil/lib/subnormal_config.cl
1 ↗(On Diff #255494)

It was copied and then just had its functionality decimated. I don't know what's going on with the copyright year, though. Should I just resubmit with the header reconciled?

tstellar added inline comments.May 22 2020, 9:56 AM
libclc/spirv-mesa3d-dxil/lib/subnormal_config.cl
1 ↗(On Diff #255494)

So git tracked the copy? I think it would be less confusing if you updated the patch, so that this is just a new file.

daniels marked an inline comment as done.May 22 2020, 10:18 AM
daniels added inline comments.
libclc/spirv-mesa3d-dxil/lib/subnormal_config.cl
1 ↗(On Diff #255494)

Yeah, git just does that under the scenes. It's not an explicit request, but it helpfully finds that for you. Even if you somehow break through that, Phabricator will also automatically reconstruct it for you too. I don't think there's a practical way to subvert this.

tstellar accepted this revision.May 27 2020, 7:35 AM

LGTM.

This revision is now accepted and ready to land.May 27 2020, 7:35 AM
jenatali added inline comments.
libclc/spirv-mesa3d-dxil/lib/SOURCES
2 ↗(On Diff #255494)

We should add ../../generic/lib/async/async_work_group_strided_copy.cl and ../../generic/lib/async/wait_group_events.cl here too.

jvesely added inline comments.Jun 4 2020, 8:14 AM
libclc/CMakeLists.txt
101

automagic build targets are not nice, because they can fail silently.
I don't mind having dxil in ALL, but an error check if the user selected dxil and llvm-spirv tools were not found would be useful.

257

This path is unnecessarily duplicating a lot of setup form none path.
Moving the spirv special case in a separate condition block below and only setting the extra flags would be a lot cleaner with fewer duplicated lines.

261

"-DCLC_${arch}" can be added globally for all targets.

263

build_flags and opt_flags would be more readable.

294

${dflags} should be in target_compile_definitions rather than target_compile_options

jenatali requested changes to this revision.Jun 4 2020, 8:19 AM

It looks like we also need to add _CLC_OVERLOAD to the declaration and definitions of the functions declared in workitem as well as barrer(). The LLVM->SPIR-V converter requires these function names to be mangled in order to appropriately translate them to the dedicated SPIR-V built-ins. The names are not mangled for C/CLC unless overloading is requested.

This revision now requires changes to proceed.Jun 4 2020, 8:19 AM
daniels retitled this revision from libclc: Add Mesa/SPIR-V/DXIL target to libclc: Add Mesa/SPIR-V target.Jun 16 2020, 5:24 AM
daniels edited the summary of this revision. (Show Details)
daniels marked 5 inline comments as done.EditedJun 16 2020, 6:09 AM

It looks like we also need to add _CLC_OVERLOAD to the declaration and definitions of the functions declared in workitem as well as barrer(). The LLVM->SPIR-V converter requires these function names to be mangled in order to appropriately translate them to the dedicated SPIR-V built-ins. The names are not mangled for C/CLC unless overloading is requested.

I'll submit that as a separate change. I don't have a good way to test Clover at the moment, so I have to dig out the relevant Clover/llvmpipe branches and get them built.

libclc/CMakeLists.txt
101

We already have the same for amdgcn-mesa-mesa3d, which won't be available unless LLVM >= 3.9 is used. So would you like me to always add the SPIR-V targets to all, then fail the build if the SPIR-V tools are not present but the user has selected (either through all or explicitly) to build the SPIR-V targets?

daniels updated this revision to Diff 271075.Jun 16 2020, 6:10 AM

Updated after review

jenatali accepted this revision.Jun 16 2020, 7:00 AM
This revision is now accepted and ready to land.Jun 16 2020, 7:00 AM
jvesely accepted this revision.Jun 16 2020, 2:09 PM

thanks!

daniels updated this revision to Diff 271313.Jun 17 2020, 2:51 AM

Rebased on top of master

daniels updated this revision to Diff 271314.Jun 17 2020, 2:53 AM

clang-format changes, seemingly silently generated by arc lint?

Hmm, another new failure, this time because clang-tidy is newly running on the CL source files, and doesn't like them. Should I just blacklist all the libclc source files?

PR to make clang-tidy not try to parse CLC source: https://github.com/google/llvm-premerge-checks/pull/202

That PR was merged to fix the build failure, and now we have a new failure ... https://github.com/google/llvm-premerge-checks/issues/207

Third time's a charm - the CI now passes. Can someone please push this when you're ready?

bbrezillon added inline comments.
libclc/CMakeLists.txt
274

Looks like the --spirv-ocl-builtins-version has been renamed to --spirv-target-env (see https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/667bf137eeda7af93b482552c8959376e9565fc1#diff-8b7efcbcbffdd234c43aeda3bb73eaa5), and AFAICT, this should have been --spirv-ocl-builtins-version="CL1.2".
Maybe we can just drop this option since CL1.2 seems to be the default anyway.

daniels updated this revision to Diff 285380.Aug 13 2020, 8:20 AM

Removed builtins version per @bbrezillon, rebase

daniels marked an inline comment as done.Aug 13 2020, 8:22 AM

I've rebased this against current master whilst fixing @bbrezillon's comment.

@tstellar @jvesely Can one of you please land this, D83473: libclc: Fix FP_ILOGBNAN definition, and D82078: libclc: Make all built-ins overloadable? We are ready to merge the SPIR-V libclc support into Mesa for multiple drivers now, but are blocking on this being merged into LLVM trunk.

libclc/CMakeLists.txt
274

Updated, thanks!

This revision was automatically updated to reflect the committed changes.