This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Generate struct type for sampler_t and function call for the initializer
ClosedPublic

Authored by yaxunl on Jun 21 2016, 1:02 PM.

Details

Summary

Currently Clang use int32 to represent sampler_t, which have been a source of issue for some backends, because in some backends sampler_t cannot be represented by int32. They have to depend on kernel argument metadata and use IPA to find the sampler arguments and global variables and transform them to target specific sampler type.

This patch uses opaque pointer type opencl.sampler_t* for sampler_t. For each use of program-scope sampler variable, it generates a function call of __translate_sampler_initializer. For each initialization of function-scope sampler variable, it generates a function call of __translate_sampler_initializer. For example,

constant sampler_t s1 = X1; // X1 is an integer literal
 
void f() {
  f1(s1);
  f2(s1);
  const sampler_t s2 = X2; // X2 is an integer literal
  f3(s2);
  f4(s2);
}

=> Llvm bitcode equivalent to (assuming opencl.sampler_t is the opaque type to represent sampler_t):

// opaque struct type for sampler
struct opencl.sampler_t;

constant opencl.sampler_t *__attribute__((always_inline)) __translate_sampler_initializer(unsigned); // a builtin function for translating sampler literal
 
void f() {
  f1(__translate_sampler_initializer(X1));
  f2(__translate_sampler_initializer(X1));
  constant __opencl_sampler_t *s2 = __translate_sampler_initializer(X2);
  f3(s2);
  f4(s2);
}

Each builtin library can implement its own __translate_sampler_initializer(). Since the real sampler type tends to be architecture dependent, allowing it to be initialized by a library function simplifies backend design. A typical implementation of __translate_sampler_initializer could be a table lookup of real sampler literal values. Since its argument is always a literal, the returned pointer is known at compile time and easily optimized to finally become some literal values directly put into image read instructions.

The advantage of this representation is:

  1. Robust - can be optimized without losing information
  2. Easy to implement – can be implemented by library instead of pass

The implementation of the design is to introduce a opaque sampler type and translates each sampler variable to sampler type in AST, and introduces cast from integer to sampler for each variable initializer. For copy initialization with function-scope variable, create integer to sampler cast. In codegen, sampler type is translated to opaque opencl.sampler_t* type. The cast from integer to sampler is translated to function call __translate_sampler_initializer. The global variable of sampler type is not translated since they are no longer referenced.

This patch is partially based on Alexey Sotkin's work in Khronos Clang (https://github.com/KhronosGroup/SPIR/commit/3d4eec61623502fc306e8c67c9868be2b136e42b).

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 61422.Jun 21 2016, 1:02 PM
yaxunl retitled this revision from to [OpenCL] Generate struct type for sampler_t and function call for the initializer.
yaxunl updated this object.
yaxunl added reviewers: Anastasia, bader, pxli168.
yaxunl added subscribers: cfe-commits, tstellarAMD.
yaxunl updated this object.Jun 21 2016, 1:19 PM
yaxunl updated this revision to Diff 61443.Jun 21 2016, 2:07 PM

Add test deleted by accident.

Anastasia added inline comments.Jun 23 2016, 7:10 AM
include/clang/AST/BuiltinTypes.def
164 ↗(On Diff #61443)

I can't get why is this necessary to represent initializer as a special Clang type? Could we avoid this additional complexity?

include/clang/AST/OperationKinds.def
328

Could we just have only int->sampler conversion? Without having an extra type for initializer?

include/clang/Basic/DiagnosticGroups.td
876

I don't understand the description really? Why not compatible?

include/clang/Driver/CC1Options.td
690 ↗(On Diff #61443)

Any reason to have this flag and support different sampler representations in Clang?

lib/CodeGen/CGOpenCLRuntime.cpp
52

Could you please keep coherency in type naming i.e. add "opencl." prefix.

yaxunl added inline comments.Jun 23 2016, 1:04 PM
include/clang/AST/BuiltinTypes.def
164 ↗(On Diff #61443)

If we don't change the type in AST, we need to translate the sampler type in AST to different types based on whether the entry is a variable or a function argument. This needs some ugly hacking of the codegen.

include/clang/AST/OperationKinds.def
328

If we don't insert the sampler initializer to sampler cast in AST, when we want to insert the function call __translate_sampler_initializer in codegen, we have to hacking the generic translation of function call instruction and other instructions which may have a sampler variable as operand to insert the function call, which would be ugly.

Or we could do a post-processing of the llvm module at the end of codegen to insert the function calls, if this approach is acceptable.

include/clang/Basic/DiagnosticGroups.td
876

e.g. the sampler constant may take some literal value which is not compatible with SPIR/SPIR-V spec. A warning will be emitted. Such warning msg belongs to this category.

include/clang/Driver/CC1Options.td
690 ↗(On Diff #61443)

Give user some time for transition.

lib/CodeGen/CGOpenCLRuntime.cpp
52

We need to be able to implement function

__sampler* __translate_sampler_initializer(__sampler_initializer*);

in library, where __sampler and __sampler_initializer are concrete struct types defined in the library source code. Therefore we cannot have dot in the struct type name.

Anastasia added inline comments.Jun 24 2016, 12:22 PM
include/clang/AST/BuiltinTypes.def
164 ↗(On Diff #61443)

I don't understand why this is needed. Surely, sampler type is just a sampler type independently from the context it's being used. Could you elaborate or give me an example please.

include/clang/AST/OperationKinds.def
328

I don't understand why this is needed? The initialization function only replaces the initializer of a sampler type. So this call only has to be emitted on the conversion int->sampler.

include/clang/Basic/DiagnosticGroups.td
876

Could we avoid having SPIR incompatible code instead?

include/clang/Driver/CC1Options.td
690 ↗(On Diff #61443)

It doesn't look such a big and risky change to add special flag and maintain two different representations. I would rather go for cleanup of old code here.

lib/CodeGen/CGOpenCLRuntime.cpp
52

Would it work if this function uses OpenCL original types and get compiled with Clang too?

yaxunl added inline comments.Jul 6 2016, 7:51 AM
include/clang/AST/BuiltinTypes.def
164 ↗(On Diff #61443)

For example,

constant sampler_t a = 0;
kernel void f(sampler_t b) {
   g(a);
   g(b);
 }

Let's say we don't introduce a OCLSamplerInitTy in AST. We only introduce OCLSamplerTy to represent sampler_t in AST. Then both variables a and b have OCLSamplerTy type in AST.

In codegen, type translation is done by CodeGenTypes::ConvertType, which only accepts a QualType argument, which means we can only translate an AST type to an LLVM type based on the AST type itself. Therefore we can only translate OCLSamplerTy to one LLVM type. Let's say we translate OCLSamplerTy to __sampler* type. Then variable a will be translated to __sampler* type. However, we want it to be translated to __sampler_initializer* type. To do that, we have to customize CodeGenModule::EmitGlobalVarDefinition to translate global variables with OCLSamplerTy type specially, which will be ugly.

Basically we cannot avoid the complexity of translating the type for variable a. The complexity is only moved from AST to Codegen. If we introduce OCLSamplerInitTy in AST, we don't need to translate the type of variable a specially. It seems to be a more elegant solution to me.

include/clang/AST/OperationKinds.def
328

Let's consider this example:

constant sampler_t a = 0;
kernel void f() {
   g(a);
 }

In codegen, we want to insert a call of __transform_sampler_initializer for the reference of variable a, not the definition of variable a. If we introduce a cast SamplerInitializerToSamplerCast, we can represent that in AST equivalent to:

__sampler_initializer *a = IntToSamplerInitializerCast(0);
kernel void f() {
   g(SamplerInitializerToSamplerCast(a));
}

We can do a straight forward translation by translating SamplerInitializerToSamplerCast to function call of __transform_sampler_initializer.

If we do not have SamplerInitializerToSamplerCast, the AST will be equivalent to:

__sampler *a = IntToSamplerCast(0);
kernel void f() {
   g(a);
}

Can we translate IntToSamplerCast to __transform_sampler_initializer? No. Because translation of IntToSamplerCast is done at translating global variable a, which happens before translating instructions belonging to a function, whereas we need to insert a function call of __transform_sampler_initializer when translating function call of g. Since we do not have a landmark for inserting function call of __transform_sampler_initializer, we have to customize CodeGenFunction::EmitCall to handle argument being a global variable of type __sampler specially, which will be ugly.

However we do have a simple solution for this if we do a post-processing of the generated LLVM IR in CodeGenModule::Release. Basically we iterate all uses of global variable a and replace them with a function call of __transform_sampler_initializer. By doing this we can use a simple AST without introducing the sampler initializer type and the cast from sampler initializer to sampler. Since CodeGenModule::Release already contains quite a few post-prcessing's for LLVM IR, I think it is reasonable to do some OpenCL specific post-processing there. We can introduce a new member CGOpenCLRuntime::postprocessIR for this purpose.

include/clang/Basic/DiagnosticGroups.td
876

It is the user's OpenCL source code which may contain SPIR incompatible integer literal value for sampler, e.g. a user may write

sampler_t s = 0;

This will result in a sampler value not compatible with SPIR. We cannot prevent user from doing that, but we can emit a warning for that.

include/clang/Driver/CC1Options.td
690 ↗(On Diff #61443)

I can remove the support of old sampler representation.

lib/CodeGen/CGOpenCLRuntime.cpp
52

Probably not. The OpenCL type sampler_t is translated to a pointer to an opaque struct. However we need to return a pointer to a concrete struct type since we need to fill the struct in __transform_sampler_initializer.

yaxunl updated this revision to Diff 62966.Jul 6 2016, 2:18 PM

Removed the old representation of sampler type by i32.

Anastasia added inline comments.Jul 7 2016, 8:28 AM
include/clang/AST/BuiltinTypes.def
164 ↗(On Diff #62966)

However, we want it to be translated to __sampler_initializer* type.

This is precisely what I am not able to understand. Why do we want it to be __sampler_initializer* type? I don't think we want that. If it was a sampler type originally in OpenCL, it should be kept a sampler type in LLVM too.

include/clang/AST/OperationKinds.def
328

In codegen, we want to insert a call of __transform_sampler_initializer for the reference of variable a, not the definition of variable a.

I think the original discussion was to insert the call on the declaration of sampler variable as an initializer. This would allow us to use variable itself "as is" everywhere else.

So if let's say we compile:

constant sampler_t a = 0;
kernel void f() {
   g(a);
}

the code we are generating would be equivalent to:

kernel void f() {
  __sampler* a = __transform_sampler_initializer(0);
  g(a);
}

Why are we not using this approach?

I think we discussed later that we could generate a struct of initializers instead of just int (0 -> struct sampler_initializer). Here is the description: http://lists.llvm.org/pipermail/cfe-dev/2016-June/049389.html
But general principle still was as described above...

include/clang/Basic/DiagnosticGroups.td
876

I see. This is just because OpenCL doesn't allocate actual constants for different allowed sampler modes whereas SPIR does.

lib/CodeGen/CGOpenCLRuntime.cpp
52

If you do conversion struct ptr <-> opaque ptr in this function, it should be fine? I would imagine images and other OpenCL types have the same requirement.

yaxunl added a subscriber: yaxunl.Jul 7 2016, 10:47 AM

+ Brian

Hi Anastasia,

The advantage for translating sampler variable to a global variable with __sampler_initializer type is that it is consistent with OpenCL C++ and SPIRV, so it is easier to translate the IR to SPIRV.

About the type name of sampler_t in LLVM, using a name without . allows library functions to use the concrete sampler types directly without casting. Casting not only affects the readability of the library code, but also causes extra efforts in optimizing these codes.

Sam

yaxunl updated this revision to Diff 63286.Jul 8 2016, 12:34 PM
yaxunl updated this object.

Drop the sampler initializer structure as Anastasia suggested. Now __translate_sampler_initializer accepts an integer parameter.

Anastasia added inline comments.Jul 11 2016, 10:17 AM
lib/CodeGen/CodeGenModule.cpp
4309

Seems like this code is specific to SPIR and not OpenCL?

lib/Sema/SemaInit.cpp
6907

Is the assert no longer needed?

6909

Debug output?

test/CodeGenOpenCL/sampler.cl
24

Why does the initialization happen second time here?

27

Could we check definition of global sampler variable in IR too?

Anastasia added inline comments.Jul 11 2016, 10:17 AM
include/clang/Basic/DiagnosticSemaKinds.td
7868

start in lower case as all other messages!

lib/CodeGen/CGExprConstant.cpp
694

Does this only apply to global variable samplers?

yaxunl marked 20 inline comments as done.Jul 11 2016, 10:56 AM
yaxunl added inline comments.
lib/CodeGen/CGExprConstant.cpp
694

Yes. The translation of local sampler is like

sampler_t p = X;

->

__sampler** p = alloca __sampler*;
store __translate_sampler_initializer(X), p
lib/CodeGen/CodeGenModule.cpp
4309

This warning msg is emitted for SPIR incompatible sampler value. It belongs to a category spir-compat. How about we turn off this category of warning by default and only enable it for SPIR target by default?

lib/Sema/SemaInit.cpp
6907

This assert is unlikely to happen. not so useful.

test/CodeGenOpenCL/sampler.cl
24

We cannot call function in the initializer of an LLVM global variable, so each reference of the global variable is replaced by a call of __transform_sampler_initializer and the original global variable is eliminated.

27

The original global variable is eliminated and its references are replaced by call of __transform_sampler_initializer.

yaxunl updated this revision to Diff 63556.Jul 11 2016, 12:58 PM
yaxunl marked 3 inline comments as done.

Revised by Anastia's comments. Disabled warning for sampler enum value by default.

yaxunl updated this revision to Diff 63571.Jul 11 2016, 2:06 PM

Added test for invalid sampler values.

Anastasia added inline comments.Jul 12 2016, 9:20 AM
lib/CodeGen/CGOpenCLRuntime.cpp
83

I am wondering if we could at least name it __opencl_sampler_t. I kind of don't like that we have inconsistency with the rest here.

Btw, the types in Debug are all named opencl_<type>. Perhaps we could correct them consistently across?

lib/CodeGen/CodeGenModule.cpp
4309

Yes sounds good!

lib/Sema/SemaInit.cpp
6907

But I am guessing that holds for most of the assertions. :)

6915

Should you also check for type of Expr to be sampler?

Btw is this covered in testing?

test/CodeGenOpenCL/sampler.cl
1

I think we can give this warning for spir target or -Wspir-compat.

17

This should go into SemaOpenCL tests!

27

Could we add CHECK-NOT respectively?

37

Can we add another call with smp and check that __translate_sampler_initializeris is not called again?

yaxunl marked 11 inline comments as done.Jul 13 2016, 1:34 PM
yaxunl added inline comments.
lib/CodeGen/CGOpenCLRuntime.cpp
83

I will rename it as __opencl_sampler_t.

I would suggest to rename all opencl types as __opencl_x instead of opencl.x (in another patch) to avoid unnecessary casts in libraries.

lib/Sema/SemaInit.cpp
6907

I can add it back.

6915

The type of Init is SourceType. We already checked it in line 6912.

Here we want to handle the case where a sampler type variable is passed as function argument. Since DRE->getDecl() has the same type as DRE, so we don't need to check that.

Since we don't allow operators on sampler, the only sampler expressions we can get are

  1. sampler type global variable
  2. sampler type function-scope variable
  3. sampler type function parameter

I covered the 3 cases in the tests. Also covered passing non-sampler type expression as sampler argument to a function. However I noticed I did not cover the cases that sampler type variable without initializer. I will add tests for that.

test/CodeGenOpenCL/sampler.cl
1

Sorry I did not turn on the warning for spir-compat by default for SPIR target. As I checked the part of code about warnings, I did not find a precedence to turn on warning for a specific target? So here I still need -Wspir-compat to see the warnings for SPIR target. Should I turn on the warning for spir-compat by default for SPIR target.

17

This warning is emitted at codegen stage instead of parsing since diagnosing this requires evaluated value of the initializer expression, which is readily available at codegen, but need some extra effort to obtain at parsing.

One side effect is that we will only see the warning if the test does not contain other sema errors because otherwise we will not reach codegen stage. That's why I test it in a codegen test since the sema tests contain other errors.

27

Will do.

37

Will do. Currently it generates call for each reference of smp, but I will fix that.

yaxunl updated this revision to Diff 63897.Jul 13 2016, 7:00 PM
yaxunl updated this object.
yaxunl marked 3 inline comments as done.

Revised by Anastasia's comments. This fixes multiple function call of __translate_sampler_initializer for function-scope sampler variables.

yaxunl updated this revision to Diff 64154.Jul 15 2016, 9:53 AM

Turn on -Wspir-compat for spir target by default. Diagnose invalid sampler value at Sema instead of Codegen.

yaxunl marked 11 inline comments as done.Jul 15 2016, 9:55 AM
yaxunl added inline comments.
test/CodeGenOpenCL/sampler.cl
3

Fixed and moved to sema test.

19

Moved to sema tests.

Anastasia added inline comments.Jul 19 2016, 7:55 AM
lib/CodeGen/CodeGenModule.cpp
2399

Could we lift this check right to the beginning of the function?

lib/Sema/SemaInit.cpp
6915–6922

It would be nice to put some comments here summarizing your description!

Btw, I don't think it's covered in testing yet, isn't it?

6916

I don't get why this level of indirection is added? Should the variable initialization be handled elsewhere?

6920

CurInit.get() can be changed to Init

test/SemaOpenCL/sampler_t.cl
21

Why was this change required?

yaxunl updated this revision to Diff 64529.Jul 19 2016, 11:24 AM
yaxunl marked 7 inline comments as done.

Revised by Anastasia's comments.

lib/Sema/SemaInit.cpp
6915–6922

Added detailed comments.

I only missed passing sampler parameter as function call argument. Now added.

6916

For global variables, since we cannot initialize them with a function call __translate_sampler_initializer, we have to take their initializer and pass them to a function call.

test/SemaOpenCL/sampler_t.cl
21

Since i added a run with spir triple, there is an extra error msg emitted:

function with no prototype cannot use the spir_function calling convention

adding void argument suppresses this error msg.

Anastasia added inline comments.Jul 20 2016, 7:45 AM
lib/Sema/SemaInit.cpp
6915–6933

Is this being tested anywhere:

sampler_t initialization requires 32-bit integer
6928

What if global variable sampler is initialized with another sampler variable:

sampler_t s1 = ...;
sampler_t s2 = s1;
...
foo(s2);

Btw, I am wondering whether this code is needed at all, because I am guessing variable initialization will be handled separately anyways, irrespective to whether it's being used in a call or not...

yaxunl added inline comments.Jul 20 2016, 7:59 AM
lib/Sema/SemaInit.cpp
6928

clang currently does not allow assigning a sampler global variable with another sampler global variable. An error will be emitted:

initializer element is not a compile-time constant

We could allow this, but I think allowing this is not a very useful feature, so I would recommend continue not allowing it.

If we do not replace references of function-scope sampler variable with its initializer here, we will end up with a global variable initialized with an integer and we will not be able to do codegen for it as in LLVM we cannot initialize a global variable with a function call.

yaxunl updated this revision to Diff 64724.Jul 20 2016, 12:19 PM

Added more diagnostic tests.

yaxunl updated this revision to Diff 64927.Jul 21 2016, 11:12 AM
yaxunl marked 4 inline comments as done.

Update the test to accommodate changes in branch tip.

Anastasia added inline comments.Jul 26 2016, 3:42 AM
lib/Sema/SemaInit.cpp
6928

Could you please take a look at the new diagnostics you are adding in DiagnosticSemaKinds.td. I don't think they are all covered by your tests.

I understand that you have to insert the cast even though it looks a bit hacky, so we might need to put more comments here to explain why this is needed. But certainly diagnostics code seems to be doing extra work. For the following code:

constant sampler_t glb_smp = 1L;
void foo(sampler_t);
void bar(){
  foo(glb_smp);
}

I get diagnostics reported twice:

sampler.cl:1:20: error: sampler_t initialization requires 32-bit integer, not 'long'
constant sampler_t glb_smp = 1L;
sampler.cl:6:5: error: sampler_t initialization requires 32-bit integer, not 'long'
foo(glb_smp);

The second diagnostic doesn't seem right at all.

Could you please take a look.

yaxunl updated this revision to Diff 65588.Jul 26 2016, 1:29 PM
yaxunl marked an inline comment as done.

Add more sema tests. Eliminate redundant diagnostic messages.

Anastasia edited edge metadata.Jul 27 2016, 10:57 AM

Btw, I am missing tests for generated __translate_sampler_initializer which I think we had at some point.

lib/CodeGen/CGOpenCLRuntime.cpp
83

Related to this thread: http://lists.llvm.org/pipermail/cfe-dev/2016-July/050126.html.

I am guessing leaving the old naming scheme should be fine.

lib/Sema/SemaInit.cpp
6907

There are 5 cases it seems :)

6929

beend -> been

6931

Do we accept declaring global sampler_t variable without an initializer?

6938

Could you use else here instead of adding TakeLiteral variable to avoid going to the next compound statement?

6947

Should this generate an error?

test/SemaOpenCL/sampler_t.cl
15

Are these extra calls below adding any extra testing?

21

Does this line add extra testing?

yaxunl marked 9 inline comments as done.Jul 27 2016, 1:18 PM
yaxunl added inline comments.
lib/CodeGen/CGOpenCLRuntime.cpp
83

I have some more comments about that. I think we should use struct.__opencl_sampler as the name of sampler type in LLVM IR.

lib/Sema/SemaInit.cpp
6931

no. Since in file scope sampler variable is only allowed in constant addr space, it has to be initialized. There is a test for it.

6938

Good catch. Will do.

6947

This has already been diagnosed when parsing the intializer of the variable declaration.

test/SemaOpenCL/sampler_t.cl
15

Add the function calls to test

  1. No additional error msgs since the error msgs should be emitted for the variable declarations.
  2. no crashes when parsing them
21

Test no crashes when an invalid sampler expression is passed as a sampler argument.

yaxunl updated this revision to Diff 65796.Jul 27 2016, 1:20 PM
yaxunl edited edge metadata.
yaxunl marked 5 inline comments as done.

Revised by Anastasia's comments. Added missing codegen test.

yaxunl updated this revision to Diff 65928.Jul 28 2016, 7:29 AM
yaxunl updated this object.

Use 'opencl.sampler_t' as opque type name for sampler in LLVM IR.

Anastasia accepted this revision.Jul 28 2016, 10:05 AM
Anastasia edited edge metadata.

LGTM! Thanks! Could you please address these small comments before committing!

lib/Sema/SemaInit.cpp
6942

Can you remove this sentence as it duplicates the previous one.

6944

I think you don't need this check any more because this code is inside the else part. Could you please double check before committing?

test/CodeGenOpenCL/sampler.cl
4

5 cases

This revision is now accepted and ready to land.Jul 28 2016, 10:05 AM
yaxunl marked 2 inline comments as done.Jul 28 2016, 10:25 AM
yaxunl added inline comments.
lib/Sema/SemaInit.cpp
6944

I tried removing this line. There will be extra diagnostics emitted for test case:

constant sampler_t glb_smp6 = glb_smp;

We only expects error msg: initializer element is not a compile-time constant. However in addition to that, we also get sampler_t initialization requires 32-bit integer, not '__constant sampler_t'. This is because the break is removed. So I think better not removed the line.

Anastasia added inline comments.Jul 28 2016, 10:36 AM
lib/Sema/SemaInit.cpp
6944

Sure! Thanks for checking it!

This revision was automatically updated to reflect the committed changes.
yaxunl marked an inline comment as done.