KhronosGroup / KhronosGroup/SPIRV-LLVM-Translator

Wrong atomic instructions translation from SPIRV to LLVM [OpenCL 1.2]

Open
#266 4 comments 0 reactions 0 assignees View on GitHub
Dominant language
LLVM
Stars
625
Forks
279
Avg merge
3d 5h
Merged PRs (30d)
34

Description

Currently all spirv atomic instructions are translated into OpenCL 2.0 llvm builtins, despite of the value specified in `-spirv-ocl-builtins-version`.

Let's consider that OpenCL 1.2 kernel is translated from LLVM to SPIRV and then back to LLVM.
LLVM: `atomic_add(global or local ptr)` --> SPIRV: `OpAtomicIAdd ` --> LLVM: `fetch_add_explicit(global or local ptr)`
The first issue here is that translator produces OpenCL 2.0 version of function and the second (most important) is that this builtin is not even OpenCL-consistent, because fetch_add_explicit takes ptr with generic address space, not global or local.

I've resolved this issue for almost all atomic instructons (https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/263) except OpAtomicLoad, OpAtomicStore, OpAtomicFlagTestAndSet and OpAtomicFlagClear.

How should we translate f.eg. OpAtomicLoad for OpenCL 1.2? There is no counterpart instruction for this one in OpenCL 1.2. Should we issue an assert if SPIRV module contains OpAtomicLoad and `CL1.2` is specified by user? I think it's incorrect, because OpAtomicLoad is not specified as not supported in OpenCL 1.2 validation rules (https://www.khronos.org/registry/OpenCL/specs/2.2/html/OpenCL_Env.html#_validation_rules_5). Maybe we should somehow emulate this instruction with regular OpenCL 1.2 load instruction (not atomic).

I've opened this issue to discuss the best, proper way of translating those four instructions for OpenCL 1.2. Would be great to hear what's your opinion about that.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing the existing atomic-instruction changes in PR #263 and the handling selected by `-spirv-ocl-builtins-version`. Focus on `OpAtomicLoad`, `OpAtomicStore`, `OpAtomicFlagTestAndSet`, and `OpAtomicFlagClear` for OpenCL 1.2, and consult the linked OpenCL validation rules. Done means a decided, consistent translation or rejection policy for all four instructions.

Written by the indexing model from the issue text.

Assessment

Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.