Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[OpenCL] Fixed missing address space for templated copy constructor

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



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.Jul 17 2020, 10:24 AM

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.Jul 20 2020, 4:08 AM
olestrohm added inline comments.

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.Jul 20 2020, 4:09 AM
Anastasia added inline comments.Jul 20 2020, 10:20 AM

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.Jul 21 2020, 4:48 AM

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

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

LGTM! Thanks

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