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

[bader]

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.

[yaxunl]

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]

test/CodeGenOpenCL/sampler.cl

62

AFAIK, we have diagnostics for both:

• declaration of a pointer to sampler
• taking address of sampler variable

[yaxunl]

LGTM. Thanks.

[Anastasia]

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]

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?

[Anastasia]

In D34342#785454, @bader wrote:

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]

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]

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?

[yaxunl]

In D34342#786941, @bader wrote:

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?

[bader]

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.

[Anastasia]

In D34342#792350, @bader wrote:

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]

LGTM! Thanks!

lib/CodeGen/CGDecl.cpp

void CodeGenFunction::EmitVarDecl(const VarDecl &D) {
   if (D.hasExternalStorage())
     // Don't emit it now, allow it to be emitted lazily on its first use.
     return;
 
   // Some function-scope variable does not have static storage but still
   // needs to be emitted like a static variable, e.g. a function-scope
   // variable in constant address space in OpenCL.
   if (D.getStorageDuration() != SD_Automatic) {
+    // Static sampler variables translated to function calls.
+    if (D.getType()->isSamplerT())
+      return;
+
     llvm::GlobalValue::LinkageTypes Linkage =
         CGM.getLLVMLinkageVarDefinition(&D, /*isConstant=*/false);
 
     // FIXME: We need to force the emission/use of a guard variable for
     // some variables even if we can constant-evaluate them because
     // we can't guarantee every translation unit will constant-evaluate them.
 
     return EmitStaticVarDecl(D, Linkage);

test/CodeGenOpenCL/sampler.cl

kernel void foo(sampler_t smp_par) {
   // Case 1c
   fnc4smp(smp_par);
   // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[smp_par_ptr]]
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
 
   fnc4smp(5);
   // CHECK: [[SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 5)
   // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
+
+  const sampler_t const_smp = CLK_ADDRESS_CLAMP_TO_EDGE | CLK_NORMALIZED_COORDS_TRUE | CLK_FILTER_LINEAR;
+  // CHECK: [[CONST_SAMP:%[0-9]+]] = call %opencl.sampler_t addrspace(2)* @__translate_sampler_initializer(i32 35)
+  // CHECK: store %opencl.sampler_t addrspace(2)* [[CONST_SAMP]], %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR:%[a-zA-Z0-9]+]]
+  fnc4smp(const_smp);
+  // CHECK: [[SAMP:%[0-9]+]] = load %opencl.sampler_t addrspace(2)*, %opencl.sampler_t addrspace(2)** [[CONST_SMP_PTR]]
+  // CHECK: call spir_func void @fnc4smp(%opencl.sampler_t addrspace(2)* [[SAMP]])
 }