Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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?

lib/CodeGen/CodeGenModule.cpp
4309

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

lib/Sema/SemaInit.cpp
6911

Debug output?

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. :)

6917–6929

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

Btw is this covered in testing?

test/CodeGenOpenCL/sampler.cl
2

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

18

This should go into SemaOpenCL tests!

28

Could we add CHECK-NOT respectively?

38

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.

6917–6929

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
2

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.

18

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.

28

Will do.

38

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
2

Fixed and moved to sema test.

18

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
6917–6929

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?

6918

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

6922

CurInit.get() can be changed to Init

test/SemaOpenCL/sampler_t.cl
55

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
6917–6929

Added detailed comments.

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

6918

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
55

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
6917–6959

Is this being tested anywhere:

sampler_t initialization requires 32-bit integer
6930

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
6930

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
6930

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 :)

6931

beend -> been

6933

Do we accept declaring global sampler_t variable without an initializer?

6940

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

6949

Should this generate an error?

test/SemaOpenCL/sampler_t.cl
35

Are these extra calls below adding any extra testing?

53

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
6933

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.

6940

Good catch. Will do.

6949

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

test/SemaOpenCL/sampler_t.cl
35

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
53

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
6944

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

6946

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
6946

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
6946

Sure! Thanks for checking it!

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