[SYCL] Fix address space assertion with templates

[premanandrao]

In certain complex template instantiations, address space mismatches
cause the compiler to assert instead of issuing errors gracefully.
This fixes one such case identified in PR #2763.

Signed-off-by: Premanand M Rao [email protected]

[premanandrao]

I would appreciate a review of the change in SemaInit.cpp primarily; if that is even the right place to address the original assertion. If the test case added (significantly reduced from the test case in #2763) could be further simplified, I would like ideas for that too.

[Fznamznon]

I'm not familiar with the code around. But the new addition it looks a bit strange. The comment above says "// Add addr space conversion if required.", but we don't allow such conversion if some of address space qualifiers were default address space and mark initialization sequence as invalid instead. Why? Could you provide a bit more details about the assertion that you are trying to fix? What is the reason for it? Why we can't cast default address space?

[premanandrao]

The case where this asserts is when trying to bind a value of type attribute((opencl_global)) X to an rvalue of the same type X.

i.e. destination is X&&, source is attribute((opencl_global)) X.

In this case, when it comes into this place where it says: '//Add addr space conversion if required.', what the code seems to be doing is first removing address space qualifiers of both the source and the destination types, doing the type conversion and then attempt to add the destination address space to the converted type. But because the destination address space is the default - LangAS::Default (no address space qualifiers), this is equivalent to throwing away the address space qualifier on the source pointer type. I think this is why it is not allowed, and why it asserts in setAddressSpace.

So rather than assert here, I changed it to a compiler error that gives more context. Does this make sense?

[Fznamznon]

Ah, it feels better now, thanks for the explanation. So, addAddressSpace asserts when we are trying to add LangAS::Default because it means "no address space" on AST level, right? The thing is, casts to "Default" address space are pretty common in our address space solution. I mean, it only works because we translate "Default" AST addr space to generic IR addr space (which is superset of all OpenCL address spaces except constant, so it is possible to cast anything to generic) and cast it whenever we need. Following the logic of our current implementation, I would say that we need to cast here to default address space as well, which likely means disabling assertion for SYCL, if we don't break any OpenCL rule or any clang logic.
@erichkeane , @bader could you please provide your opinion here? Should we diagnose or cast instead?

[bader]

I agree with @Fznamznon, that we should allow conversion to default AS here, similar to OpenCL allowing conversion to "generic".

NOTE: We are currently discussing alternative ways to implement AS conversions in different modes, but once we agree on something it would be clear if this part requires refactoring or not.

[premanandrao]

Thanks, I have removed the diagnostic message and instead do a cast for this case.