Page MenuHomePhabricator

[OpenCL] Allow OpenCL C style vector initialization in C++
ClosedPublic

Authored by Anastasia on Jul 25 2019, 8:58 AM.

Details

Summary

I am recreating the review from https://reviews.llvm.org/D59426 that seem to get lost somehow.

The idea is to be able to allow creating vector literals from other vectors.

Example from reported bug PR41010

kernel void test(__global float4 *out)
{
    float4 a = (float4)(1.0f, 2.0f, 3.0f, 4.0f);
    float4 v = (float4)(a.s23, a.s01);
    *out = v;
}

Diff Detail

Repository
rL LLVM

Event Timeline

Anastasia created this revision.Jul 25 2019, 8:58 AM
kpet added a subscriber: kpet.Jul 25 2019, 9:06 AM

Wait, plain C++? Do we currently allow this syntax outside of OpenCL?

Anastasia added a comment.EditedJul 26 2019, 4:34 AM

Wait, plain C++? Do we currently allow this syntax outside of OpenCL?

Ok I looked into this again and it seems apparently vector initialization and literals in C++ have different syntax. So I was incorrect, this fix only works for OpenCL. Not sure how important it is to initialize vectors from other vectors in C++ though. In OpenCL C it is common.

Anastasia edited the summary of this revision. (Show Details)Jul 26 2019, 4:34 AM

Okay. It seems reasonable to me to allow OpenCL vector initialization syntax in OpenCL C++, since we're treating OpenCL C++ as an ObjC++-like merge of the language extensions, but we shouldn't go further than that and start changing behavior in the non-OpenCL languages.

In vector_literals_nested.cl, we have tests for (global) constants. Do you think it would be worth testing those as well in C++ mode? Maybe the two files (vector_literals_nested.cl and vector_literals_valid.cl) should also be merged as most of their content seems duplicated.

In C++, we have the comma operator and therefore the AST is significantly different. Running the content of your test file with -x c++ shows that it is rejected as desired. It could be worth having a negative test to ensure we always reject this in vanilla C++.

Regarding the code itself, I'm not familiar with InitializationSequence/InitListChecker that much, but the patch makes sense I would say.

Anastasia updated this revision to Diff 212350.Jul 30 2019, 8:06 AM
  • Merged test/CodeGenOpenCL/vector_literals_nested.cl into test/CodeGenOpenCL/vector_literals_valid.cl
  • Added negative test case for C++
Anastasia updated this revision to Diff 212358.Jul 30 2019, 8:57 AM
  • Removed another duplicate file and added testing of constant program scope initializers into vector_literals_nested.cl

In vector_literals_nested.cl, we have tests for (global) constants. Do you think it would be worth testing those as well in C++ mode? Maybe the two files (vector_literals_nested.cl and vector_literals_valid.cl) should also be merged as most of their content seems duplicated.

Thanks for pointing out. Indeed there was lots of repetition. I only added one missing test case (line 37 of vector_literals_valid.cl) and also removed another similar file test/SemaOpenCL/vector_literals_const.cl.

In C++, we have the comma operator and therefore the AST is significantly different. Running the content of your test file with -x c++ shows that it is rejected as desired. It could be worth having a negative test to ensure we always reject this in vanilla C++.

Added test case in test/SemaCXX/vector.cpp. However I am now confused what syntax we shouldn't accept exactly. @rjmccall do you think there should be an error on line 341?

Regarding the code itself, I'm not familiar with InitializationSequence/InitListChecker that much, but the patch makes sense I would say.

Ok, thanks. It seems apparently the last time this code was modified in 2015... so might not be easy to remember details for anyone. :(

mantognini accepted this revision.Jul 30 2019, 9:26 AM

LGTM, thanks for the update.

This revision is now accepted and ready to land.Jul 30 2019, 9:26 AM

In vector_literals_nested.cl, we have tests for (global) constants. Do you think it would be worth testing those as well in C++ mode? Maybe the two files (vector_literals_nested.cl and vector_literals_valid.cl) should also be merged as most of their content seems duplicated.

Thanks for pointing out. Indeed there was lots of repetition. I only added one missing test case (line 37 of vector_literals_valid.cl) and also removed another similar file test/SemaOpenCL/vector_literals_const.cl.

In C++, we have the comma operator and therefore the AST is significantly different. Running the content of your test file with -x c++ shows that it is rejected as desired. It could be worth having a negative test to ensure we always reject this in vanilla C++.

Added test case in test/SemaCXX/vector.cpp. However I am now confused what syntax we shouldn't accept exactly. @rjmccall do you think there should be an error on line 341?

No, it's correct that this is accepted as a vector splat. The warning is good, although it would be even better if we could find a way to warn about this more specifically, like "warning: this syntax does not construct a vector; initial elements are ignored" (needs workshopping, obviously).

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 4:20 AM

Please be aware about build bot failure (http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-full-sh/builds/2185) most likely caused by this change.

Please be aware about build bot failure (http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-full-sh/builds/2185) most likely caused by this change.

Thanks! I believe it is now fixed in http://llvm.org/viewvc/llvm-project?view=revision&revision=367823.