Page MenuHomePhabricator

[OpenCL] Fixed missing address space for templated copy constructor
ClosedPublic

Authored by olestrohm on Jul 13 2020, 3:25 AM.

Details

Summary

When you have a templated constructor with an R-value reference the compiler implicitly creates a copy and move constructor through the template constructor, and the copy constructor created this way was missing an address space in the reference parameter. I've come up with this fix that deduces the address space for the reference parameter right before it's created, and a corresponding test.

The test is quite big, but rep_outer is necessary to force the compiler to create the copy constructor, and the two template structs are partly for readability.

Diff Detail

Event Timeline

olestrohm created this revision.Jul 13 2020, 3:25 AM
Anastasia added inline comments.Fri, Jul 17, 10:24 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
3820

I feel we can just add an address space explicitly since we are creating the type here for a known use case. However, does Arg actually have an address space? I am just unsure whether we should use generic address space or concrete address space.

olestrohm marked an inline comment as done.Mon, Jul 20, 4:08 AM
olestrohm added inline comments.
clang/lib/Sema/SemaTemplateDeduction.cpp
3820

No, there are no known address spaces at this point for the test case I've got. But as far as I understand the function isn't only used for the case we look at here, but there may be case where the argument has an address space.

olestrohm marked an inline comment as not done.Mon, Jul 20, 4:09 AM
Anastasia added inline comments.Mon, Jul 20, 10:20 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
3820

Ok since there are no known cases for named address spaces now, we could just use generic. We can always extend this later if we find that any other address space is needed.

olestrohm updated this revision to Diff 279485.Tue, Jul 21, 4:48 AM

The code now directly adds the __generic address space to the pointee type.

Anastasia accepted this revision.Fri, Jul 24, 5:47 AM

LGTM! Thanks

This revision is now accepted and ready to land.Fri, Jul 24, 5:47 AM
This revision was automatically updated to reflect the committed changes.