This is an archive of the discontinued LLVM Phabricator instance.

[OpenCL] Fix missing address space deduction in template variables

Authored by olestrohm on Jun 29 2020, 8:58 AM.



This patch fixes two instances of not deducing address spaces for global template variables.
I've added OpenCL address space deduction to the functions responsible for specializing template variable declarations,
and while this does give the correct address spaces, it is not optimal, as this same address space is deduced at least three times for a single variable.
It would be better if the correct type was correctly passed forward through the phases, which I've explained in the comments.
Since the variable is templated I'm not entirely certain if this is feasible or worthwhile compared to the simple solution of rededucing the address space several times.

I've also included this an example of this in the test for address space deductions

Diff Detail

Event Timeline

olestrohm created this revision.Jun 29 2020, 8:58 AM

So if I understand this correctly when the template is instantiated we don't have QualType with the address space deduced initially during parsing of a template global variable?

Essentially, in each pass the type for the variable starts off being extract from the SourceTypeInfo, but since the address space isn't specified, this type is then missing an address space.
Adding the address space deduction then reintroduces the address space to the type.

This is then done at least three times (pre-specialization, in the middle of specialization and when completing specialization), but since this is a template it may not be possible to correctly
deduce the type earlier in case the template type introduces an address space in template specialization?

Seems like you shouldn't do it earlier if the type is dependent.

olestrohm updated this revision to Diff 275345.Jul 3 2020, 3:29 AM

Added a guard in deduceOpenCLAddressSpace to stop it from deducing address space for dependent types, as requested.

olestrohm updated this revision to Diff 275351.Jul 3 2020, 3:52 AM

Disregard the last comment, rolling back that diff for now.

olestrohm updated this revision to Diff 275381.Jul 3 2020, 6:42 AM

I have added a check in deduceOpenCLAddressSpace() to check if the type is dependent, and not deduce the address space if it is. This is a big change in the behaviour of address space deduction, meaning that template variables only receive address spaces when they are being specialized. This is good, but caused a lot of changes in the current test file, which has been included in this patch.

There also had to be added a check for dependent types in CheckVariableDeclarationType when checking whether global variables have the correct address space. With this new change dependent types don't necessarily have address spaces yet, and thus then lets it pass until the variable declaration of the specialized template variable is checked later.

Anastasia added inline comments.Jul 3 2020, 11:02 AM

Is this still an issue with your new patch? I presume deducing the address space early would still be better in general but it seems not to play well with the way the template instantiation is implemented currently. Although it might generally be safer not to deduce anything for templates until the final instantiation is done.

olestrohm updated this revision to Diff 276369.Jul 8 2020, 4:27 AM
olestrohm marked an inline comment as done.

I've removed the comments calling for a fix because I no longer feel that this approach needs that. Given the code that already exists, and without changing too much of it, adding address space deduction in both cases seems like the right choice.

Anastasia accepted this revision.Jul 8 2020, 4:53 AM

LGTM! Thanks!

This revision is now accepted and ready to land.Jul 8 2020, 4:53 AM
This revision was automatically updated to reflect the committed changes.