Page MenuHomePhabricator

[C++4OpenCL] Fix reinterpret_cast of vectors
ClosedPublic

Authored by olestrohm on Apr 29 2021, 4:14 AM.

Details

Summary

This fixes two issues with reinterpret_cast in C++ for OpenCL and adds tests to make sure they both pass without errors, and generate the correct code.

Reinterpret_cast can only convert a type to itself if it is an integral type (or pointer or reference or vectors), so I didn't include any exceptions for opencl types.
Not even float can be converted to itself, so I don't think it makes sense to allow this for any opencl types unlesss you think some of them fit the integral restriction.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=47977 and https://bugs.llvm.org/show_bug.cgi?id=49221

Diff Detail

Event Timeline

olestrohm created this revision.Apr 29 2021, 4:14 AM
olestrohm requested review of this revision.Apr 29 2021, 4:14 AM
olestrohm updated this revision to Diff 341844.Apr 30 2021, 4:36 AM

Fixed formatting.

Anastasia added inline comments.
clang/lib/Sema/SemaCast.cpp
2352

I am thinking of whether we can allow something weird to be written by relaxing this address space case

local int i;
const global int & ii = reinterpret_cast<global int>(i);

I think this code is completely meaningless as it requires creating a temporary in global address space? I am not sure what we will even emit in IR?

Embedded C doesn't regulate the conversions between non-pointer types with different address spaces but in OpenCL C we have allowed them apparently https://godbolt.org/z/9f75zxj7v. I am not sure whether this was done intentionally or not.

However, in C there are no references and other features that might create problems.

At the same time it seems C++ allows casting away const https://godbolt.org/z/fo3axbq3e

But it contradicts the comment below:

// C++ 5.2.10p2 has a note that mentions that, subject to all other
// restrictions, a cast to the same type is allowed so long as it does not
// cast away constness. In C++98, the intent was not entirely clear here,
// since all other paragraphs explicitly forbid casts to the same type.
// C++11 clarifies this case with p2.

I would like to see if @rjmccall has any opinion on whether reinterpreting between non-pointer types with different address spaces is reasonable?

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp
1

Btw I assume your patch also allows reinterpret_cast between vectors? What happens if we cast b/w vector of 3 elements and vector of 4 elements?

olestrohm added inline comments.Apr 30 2021, 8:30 AM
clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp
1

Yes, this is essentially the same as line 10, as int is treated as a 1 element vector, but I can add some more test cases that shows this more specifically.

Anastasia added inline comments.May 5 2021, 5:04 AM
clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp
1

Yes, let's add a few test cases with vector to vector casts. I would especially suggest vec3 since it is treated at vec4 in storage I think...

olestrohm updated this revision to Diff 343647.May 7 2021, 4:59 AM
olestrohm retitled this revision from [C++4OpenCL] Fix reinterpret_cast of vectors and types with address spaces to [C++4OpenCL] Fix reinterpret_cast of vectors.
olestrohm set the repository for this revision to rG LLVM Github Monorepo.

Removed the address space related changes. This revision now only allows for reinterpret_cast between vectors of the same size.

Also added tests for two variables that are definitely both vectors.

This now only fixes: https://bugs.llvm.org/show_bug.cgi?id=47977

Anastasia added inline comments.May 10 2021, 5:15 AM
clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp
16

I think in OpenCL vector 3 should be setup as vector 4 in memory with the 4th component being undef value. So this conversion should be permitted.

For example you can reinterpret cast between 3 and 4 components vectors using as_typen functions.

https://godbolt.org/z/sKqnYhnGe

I don't think we have a comprehensive description but these are the quotes from OpenCL C:

For 3-component vector data types, the size of the data type is 4 * sizeof(component). This means that a 3-component vector data type will be aligned to a 4 * sizeof(component) boundary. The vload3 and vstore3 built-in functions can be used to read and write, respectively, 3-component vector data types from an array of packed scalar data type.

When the operand and result type contain a different number of elements, the result shall be implementation-defined except if the operand is a 4-component vector and the result is a 3-component vector. In this case, the bits in the operand shall be returned directly without modification as the new type.

The suffixes .lo (or .even) and .hi (or .odd) for a 3-component vector type operate as if the 3-component vector type is a 4-component vector type with the value in the w component undefined.

olestrohm updated this revision to Diff 344408.May 11 2021, 8:38 AM

I tried to add a special case for 3 and 4 element vectors, but that caused issues in codgen, so I've left it as a FIXME for now.

Anastasia added inline comments.May 12 2021, 4:46 AM
clang/lib/Sema/SemaCast.cpp
2333

Just a small nit - add . at the end.

clang/lib/Sema/SemaExpr.cpp
7347

Btw I understand that this function is copied over from the old code that has wrong formatting but since we are rewriting this now let's convert it to the current style:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

In partucular variable name should start from the upper case letter.

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp
15

Let's add a comment here that it is testing casting OpenCL types to itself.

Btw I assume casting vector to the same vector type will work? Maybe we can add a test case for this too?

At the top of the function we can add a comment about teting the vector type.

olestrohm updated this revision to Diff 345442.May 14 2021, 7:52 AM

Fixed the code style and added some comments to the tests.

Anastasia accepted this revision.May 14 2021, 7:56 AM

LGTM! Thanks!

Just remember to reflect your final change in the commit message description. :)

This revision is now accepted and ready to land.May 14 2021, 7:56 AM
This revision was automatically updated to reflect the committed changes.