LGTM. Thanks!

Hi Sam,

LGTM, but I'd like Anastasia to approve.
Thanks.

Looks good to me. Thanks for working on this.

LGTM.

LGTM. Thanks!

In D39129#923235, @Anastasia wrote:

In D39129#902848, @bader wrote:

@Anastasia, during the discussion of similar fix (https://reviews.llvm.org/D34342).

I found another bug in the CodeGen library. Do you have time to fix it too?
Here is the reproducer from the old review request:

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

I am getting an error currently: error: internal error: could not emit constant value "abstractly". It seems very weird and also doesn't seem to be related to constant AS change. I think this should be allowed to compile though? I can try to investigate but since I don't know the time frame yet and it doesn't seem to be related to constant AS I would prefer to do it as a separate change.

@Anastasia, during the discussion of similar fix (https://reviews.llvm.org/D34342).

@arichardson, great work! Thanks a lot!

Applied comments.

Fix parsing of function declarations with empty parameter list initializers.
This change should fix the failure caught by the buildbot.
Sorry for the long delay.

LGTM.

LGTM. Thanks!

In D36410#856716, @yaxunl wrote:

The captured variable is still passed by value. The address taking is on the duplicate of the captured variable, not on the original variable.

In D36410#855282, @Anastasia wrote:

Ok, I will update it to be implicitly generic then. Just to be sure, @bader do you agree on this too?

LGTM, except one nit.

In D36327#840616, @yaxunl wrote:

In D36327#839809, @rjmccall wrote:

Could you just implement this in SimplifyLibCalls? I assume there's some way to fill in TargetLibraryInfo appropriately for a platform. Is that too late for your linking requirements?

Both the optimized and generic versions of __read_pipe function contains call of other library functions and are complicate enough not to be generated programmatically. amdgpu target does not have the capability to link in library code after LLVM codegen. The linking has to be done before SimplifyLibCalls.

LGTM! Thanks.

@rsmith do you have an opinion on what would be the right place for the kind of proposed optimization?
It looks like it can be implemented as target independent optimization, acting only for target with specified properties - in this case target must provide required built-in functions.

Hi Sam,

Out of curiosity: how did you detect this?
Can we use the same approach for writing tests?

Thanks!
Overall the patch looks good, but I would suggest splitting it into three commits (as they seems to be independent):

1. [OpenCL] Check that cl_khr_subgroups pragma is enabled if respective extension is used.
2. [OpenCL] Add support for missing sub_group functions.
3. [OpenCL] Fix return type for reserve pipe built-ins.

LGTM. Thanks.

LGTM, thanks.

Do you have another built-in in mind which can use this diagnostic message?
If so, it would make sense to re-use it in the same patch.

This is split off from https://reviews.llvm.org/D33945, which I will be rebasing/re-uploading once this patch is committed.

LGTM.
Do you have another built-in in mind which can use this diagnostic message?
If so, it would make sense to re-use it in the same patch.

LGTM.
Just one minor comment.
Thanks.

@chapuni, thanks for taking care of this. I'll take a look.

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.

Although this patch is already accepted, I'd like to confirm that I can commit the latest update with changes in test/SemaOpenCL/invalid-pipes-cl2.0.cl.

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.

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.

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?

Please, split this patch into two parts:

1. Improve diagnostics on extension enabling.
2. Add missing sub_group_* built-in functions.

Applied Akira's comment: re-used cached Int32Ty type.

Update one more test missed in the first version of the patch.

Sorry for the delay.
It will also align the behavior with function definition.
For instance the following function definition is allowed.

__local int *allocateLocalMemory() { return 0; }

This issue probably can be fixed by setting SPIR target architecture or just enabling cl_khr_fp64 extension.
Unfortunately, I can't check it today.
Could you revert Egor's commit, please?

I think my topics are not related to the bug 25404 and can be handled separately.
Thanks!

Anastasia, could you generate patch with full context, please?

From all the above arguments, I feel like the right approach would be to implement it as Clang builtin which closely matches the operator semantic in my opinion. We could of course reuse the implementation of __bultin_astype to avoid unnecessary extra work and code duplication.

Using the macro seems to me more like a workaround solution and overloaded functions don't seem to be entirely the right thing either. What do you think about it?

Why do you think this is a bug? It seems to follow standard behavior in C to promote char to int if required. Just like if you would have a C code:

int as_int(int i);
void foo() {
    char src = 1;
    int dst = as_int(src);
}

This code would complie and the same exactly IR would be generated.

LGTM.
Just a few minor issues.