This is an archive of the discontinued LLVM Phabricator instance.

libclc: Add clspv64 target
ClosedPublic

Authored by kpet on Jan 5 2022, 8:37 AM.

Details

Summary

Add a variant of the clspv target that is built using spir64.
This is a pre-requisite to supporting spir64 in clspv which is
required to take advantage of SPV_KHR_physical_storage_buffer which
in turn enables more OpenCL C programs to be compiled with clspv.

Diff Detail

Unit TestsFailed

Event Timeline

kpet created this revision.Jan 5 2022, 8:37 AM
kpet requested review of this revision.Jan 5 2022, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2022, 8:37 AM
kpet updated this revision to Diff 397629.Jan 5 2022, 10:10 AM

Adding missing clspv64 symlink to the diff.

Which version of physical storage buffer do you intend to support? Because if 64-bit integers can be assumes, then the clspv64 target should use the generic implementation of fma instead of the one the standard clspv target uses to avoid all the carry bit logic.

kpet added a comment.Jan 7 2022, 1:52 AM

I've done my prototyping assuming support for 64-bit integers but, unfortunately, some targets of interest don't support 64-bit yet so I will likely end up implementing support for both paths (64-bit integers and uvec2) in clspv. I've mapped the clspv64 target to the version that doesn't require 64-bit integers for compatibility, to avoid carrying two builds of the library in clspv (each build adds about 2MB to the binary size). Looking at both implementations of fma, I'm not overly concerned about always using the version that doesn't require support for 64-bit integers for now. Both will be excruciatingly slow compared to hardware support anyway. Longer-term we can either assume support for 64-bit integers once more prevalent in the Vulkan ecosystem or introduce an additional variant of the library but I'd rather treat this as a future optimisation if that works for you.

Sounds reasonable. I agree the high precision fma is already very slow.

jvesely requested changes to this revision.Jan 8 2022, 11:15 AM

Was this tested on an existing device, or is it just for completion (since there already is clspv target)?

libclc/CMakeLists.txt
221

wrong formatting. If you spill over to the next line keep NOT close to the negated element and align to match the first line

253

formatting issue

308

just add another elseif branch for better readability. Nested if is not saving much

This revision now requires changes to proceed.Jan 8 2022, 11:15 AM
kpet updated this revision to Diff 398557.Jan 10 2022, 3:24 AM
kpet marked 2 inline comments as done.

Thanks for the review. I've now addressed the comments, PTAL.

jvesely accepted this revision.Jan 12 2022, 11:43 AM

LGTM.

thanks

This revision is now accepted and ready to land.Jan 12 2022, 11:43 AM
kpet closed this revision.Jan 13 2022, 1:31 AM
kpet marked an inline comment as done.

Thanks! Now committed.

libclc/CMakeLists.txt
308

I did it with nesting to mirror what was done for spirv and spirv64 and de-duplicate flag setting. There are fewer flags with clspv* targets though. I've removed nesting and agree it's more readable.