This is an archive of the discontinued LLVM Phabricator instance.

[C++4OpenCL] Fix initialization of __constant constructors without arguments
ClosedPublic

Authored by olestrohm on May 11 2021, 9:00 AM.

Details

Summary

This fixes the implicit initialization that uses constructors in the __constant address space without arguments.

It also adds several tests to make sure that constructors in other address spaces also work, since these have not yet been tested.

Diff Detail

Event Timeline

olestrohm created this revision.May 11 2021, 9:00 AM
olestrohm requested review of this revision.May 11 2021, 9:00 AM
Anastasia added inline comments.May 12 2021, 5:11 AM
clang/lib/Sema/SemaDecl.cpp
12643

ctor -> Ctor

clang/test/SemaOpenCLCXX/addrspace-constructors.clcpp
21

Btw, even though your change doesn't modify this I would suggest to add a CodeGen test that checks the initialization of objects in IR is adequate.

37

We seem to be missing the coverage with __constant and = default.

43

I find the wording of the error a bit odd, but it is standard C++ though...

olestrohm added inline comments.May 12 2021, 7:06 AM
clang/test/SemaOpenCLCXX/addrspace-constructors.clcpp
37

This is because of the struct W below, you can't default a constexpr constructor, but __constant constructors need to be constexpr. Though it is currently possible to create them, they just can't actually be used. Or maybe just combine Z and W?

Anastasia added inline comments.May 12 2021, 8:21 AM
clang/test/SemaOpenCLCXX/addrspace-constructors.clcpp
37

I see makes sense. I think it's ok to have separate structs. You could add a comment though to explain what you aim to test in each?

olestrohm updated this revision to Diff 345436.May 14 2021, 7:42 AM

Added a codegen test and clarified what some parts are testing.

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

LGTM! Thanks!

clang/test/CodeGenOpenCLCXX/addrspace-constructors.clcpp
22

FYI, I would remove , align 4 to simplify testing.

This revision is now accepted and ready to land.May 14 2021, 7:54 AM
olestrohm updated this revision to Diff 345845.May 17 2021, 5:44 AM

Relaxed the checks in the codegen test because of failures on platforms with slightly different IR being generated.

LGTM! Thanks!