This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Fix code generation of function-scope constant samplers.
ClosedPublic

Authored by bader on Jun 19 2017, 1:41 AM.

Details

Summary

Constant samplers are handled as static variables and clang's code generation
library, which leads to llvm::unreachable. We bypass emitting sampler variable
as static since it's translated to a function call later.

Event Timeline

bader created this revision.Jun 19 2017, 1:41 AM
yaxunl added inline comments.Jun 19 2017, 7:44 AM
test/CodeGenOpenCL/sampler.cl
62

what if address of const_smp is taken and assigned to a pointer to sampler_t ? Do we have diagnosis in place?

bader added inline comments.Jun 19 2017, 9:01 AM
test/CodeGenOpenCL/sampler.cl
62

AFAIK, we have diagnostics for both:

  • declaration of a pointer to sampler
  • taking address of sampler variable
yaxunl accepted this revision.Jun 19 2017, 9:08 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Jun 19 2017, 9:08 AM
Anastasia added inline comments.Jun 20 2017, 8:16 AM
test/CodeGenOpenCL/sampler.cl
62

Btw, strangely I don't hit any unreachable and don't seem to have any static variable generated either. I was trying to compile this code on r305798:

void fnc4smp(sampler_t s) {}
kernel void foo(sampler_t smp_par) {
  const sampler_t const_smp = 0;
  fnc4smp(const_smp);
}

This is the IR which is produced for me:

%opencl.sampler_t = type opaque

; Function Attrs: noinline nounwind optnone
define spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %s) #0 {
entry:
  %s.addr = alloca %opencl.sampler_t addrspace(2)*, align 4
  store %opencl.sampler_t addrspace(2)* %s, %opencl.sampler_t addrspace(2)** %s.addr, align 4
  ret void
}

; Function Attrs: noinline nounwind optnone
define spir_kernel void @foo(%opencl.sampler_t addrspace(2)* %smp_par) #0 !kernel_arg_addr_space !4 !kernel_arg_access_qual !5 !kernel_arg_type !6 !kernel_arg_base_type !6 !kernel_arg_type_qual !7 {
entry:
  %smp_par.addr = alloca %opencl.sampler_t addrspace(2)*, align 4
  %const_smp = alloca %opencl.sampler_t addrspace(2)*, align 4
  store %opencl.sampler_t addrspace(2)* %smp_par, %opencl.sampler_t addrspace(2)** %smp_par.addr, align 4
  %0 = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 0)
  store %opencl.sampler_t addrspace(2)* %0, %opencl.sampler_t addrspace(2)** %const_smp, align 4
  %1 = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** %const_smp, align 4
  call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* %1)
  ret void
}

declare %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32)
bader added a comment.Jun 20 2017, 9:05 AM

Wow...
Nice catch.
For some reason I can't reproduce the problem neither.
I will drop source code change, but I'd like to modify the test anyway.
https://reviews.llvm.org/rL277024 added test/CodeGenOpenCL/sampler.cl and modified test/SemaOpenCL/sampler_t.cl.
I want to move the parts of the test/SemaOpenCL/sampler_t.cl, which are supposed to pass semantic analysis to test/CodeGenOpenCL/sampler.cl and validate the LLVM IR after code generation.
Are you OK with this?

bader abandoned this revision.Jun 20 2017, 11:43 PM
Anastasia edited edge metadata.Jun 21 2017, 7:59 AM

Wow...
Nice catch.
For some reason I can't reproduce the problem neither.
I will drop source code change, but I'd like to modify the test anyway.

I think we probably had this issue at some point... Can't think of any recent commit that fixed it apart from may be r288163.

https://reviews.llvm.org/rL277024 added test/CodeGenOpenCL/sampler.cl and modified test/SemaOpenCL/sampler_t.cl.
I want to move the parts of the test/SemaOpenCL/sampler_t.cl, which are supposed to pass semantic analysis to test/CodeGenOpenCL/sampler.cl and validate the LLVM IR after code generation.
Are you OK with this?

Sure. Sounds like a good improvement to testing! Thanks!

bader added a comment.Jun 21 2017, 8:54 AM

I think I found the way to reproduce the issue. There are two pre-requisites: sampler must be declared in the constant address space and clang must be build in Debug mode.
I'll fix the test and add the test cases from test/SemaOpenCL/sampler_t.cl as mentioned earlier.

bader updated this revision to Diff 103421.Jun 21 2017, 10:39 AM

Added test case reproducing the issue described in the description.
Removed test cases from test/SemaOpenCL/sampler_t.cl covered by test/CodeGenOpenCL/sampler.cl.

While I was moving one test case from test/SemaOpenCL/sampler_t.cl, I found another bug in CodeGen library that crashes the compilation if sampler is initialized with non-constant expression.

Here is a short reproducer:

int get_sampler_initializer(void);
kernel void foo() {
  const sampler_t const_smp_func_init = get_sampler_initializer();
}

The problem is that clang does not discard this code as invalid, but CodeGen library expects sampler initializer to be a constant expression:

llvm::Value *
CodeGenModule::createOpenCLIntToSamplerConversion(const Expr *E,
                                                  CodeGenFunction &CGF) {
  llvm::Constant *C = EmitConstantExpr(E, E->getType(), &CGF); // for the reproducer expression here is CallExpr.
  auto SamplerT = getOpenCLRuntime().getSamplerType();
  auto FTy = llvm::FunctionType::get(SamplerT, {C->getType()}, false);
  return CGF.Builder.CreateCall(CreateRuntimeFunction(FTy,
                                "__translate_sampler_initializer"),
                                {C});
}

There are two ways to handle this issue:

  1. Implement diagnostics allowing only compile time constant initializers.
  2. Add non-constant sampler initializer support to CodeGen library.

OpenCL specification examples give me impression that samplers declared inside OpenCL programs must be known at compile time.
On the other hand OpenCL allows samplers passed via kernel parameters to be unknown at compile time.

Thoughts?

This revision is now accepted and ready to land.Jun 21 2017, 10:39 AM

Added test case reproducing the issue described in the description.
Removed test cases from test/SemaOpenCL/sampler_t.cl covered by test/CodeGenOpenCL/sampler.cl.

While I was moving one test case from test/SemaOpenCL/sampler_t.cl, I found another bug in CodeGen library that crashes the compilation if sampler is initialized with non-constant expression.

Here is a short reproducer:

int get_sampler_initializer(void);
kernel void foo() {
  const sampler_t const_smp_func_init = get_sampler_initializer();
}

The problem is that clang does not discard this code as invalid, but CodeGen library expects sampler initializer to be a constant expression:

llvm::Value *
CodeGenModule::createOpenCLIntToSamplerConversion(const Expr *E,
                                                  CodeGenFunction &CGF) {
  llvm::Constant *C = EmitConstantExpr(E, E->getType(), &CGF); // for the reproducer expression here is CallExpr.
  auto SamplerT = getOpenCLRuntime().getSamplerType();
  auto FTy = llvm::FunctionType::get(SamplerT, {C->getType()}, false);
  return CGF.Builder.CreateCall(CreateRuntimeFunction(FTy,
                                "__translate_sampler_initializer"),
                                {C});
}

There are two ways to handle this issue:

  1. Implement diagnostics allowing only compile time constant initializers.
  2. Add non-constant sampler initializer support to CodeGen library.

OpenCL specification examples give me impression that samplers declared inside OpenCL programs must be known at compile time.
On the other hand OpenCL allows samplers passed via kernel parameters to be unknown at compile time.

Thoughts?

The OpenCL spec v1.2 s6.9.b:

The sampler type (sampler_t) can only be used as the type of a function argument or a
variable declared in the program scope or the outermost scope of a kernel function. The
behavior of a sampler variable declared in a non-outermost scope of a kernel function is
implementation-defined. A sampler argument or variable cannot be modified.
The sampler type cannot be used to declare a structure or union field, an array of
samplers, a pointer to a sampler, or the return type of a function. The sampler type cannot
be used with the local and global address space qualifiers.

So a function returning sampler is invalid. However, the spec does not forbid a sampler
defined in the outermost scope of a kernel function to be initialized dynamically, e.g.

kernel void f(sampler_t s1, sampler_t s2, int x) {
  sampler_t s = x ? s1 : s2;
}

The question is: if we allow this, will it cause any issue?

Note: get_sampler_initializer from my test case returns integer, not a sampler, but having function is not relevant to the problem.
Here is a bit simplified test case without function calls that still reproduces the problem:

kernel void foo(int sampler_init_value) {
  const sampler_t const_smp_func_init = sampler_init_value;
}

The problem is in the function that handles sampler initialization with integer.
There should no problems with the case you provided as it doesn't require additional function call injection.

Note: get_sampler_initializer from my test case returns integer, not a sampler, but having function is not relevant to the problem.
Here is a bit simplified test case without function calls that still reproduces the problem:

kernel void foo(int sampler_init_value) {
  const sampler_t const_smp_func_init = sampler_init_value;
}

The problem is in the function that handles sampler initialization with integer.
There should no problems with the case you provided as it doesn't require additional function call injection.

This seems like a valid code which should be compiled in my view. At least I don't see anything in the spec that disallows this. This is not an interesting use case though, however it becomes more interesting with the use of ternary operator. Do you see any issues fixing the CodeGen for it?

Anastasia accepted this revision.Nov 14 2017, 9:03 AM

LGTM! Thanks!

This revision was automatically updated to reflect the committed changes.