[PATCH] D111566: [SYCL] Fix function pointer address space

[Dylan McKay via Phabricator via cfe-commits]

dylanmckay added a comment.

This will product correct code from an AVR perspective where (before this patch it would have failed codegen). It is consistent with how we construct function pointers in LLVM core as well.

I'm not very familiar with Clang internals, one thing that stick out to me:

**The logic instead belongs directly inside the existing `getTargetAddressSpace` method?**

Perhaps the new `if (is function) { set address space to the value from the data layout}` logic belongs directly inside `ASTContext::getTargetAddressSpace` or one of its descendants in the call tree. This would obviously increase the scope/possible impact of the change further. The reason I suspect it belongs in there is that in theory, if this is the correct way to get the address space for a `QualType` (which may be a function) **in this instance**, then I feel that same logic would hold for any other `QualType` that may be a function pointer that has `getTargetAddressSpace()` called on it because I don't see anything special or unique about this existing call to `getTargetAddressSpace` versus any other existing call to it inside clang. If you implement it at that lower level inside `getTargetAddressSpace`, your conditional would be something like `QualType.getTypePtr()->isFunctionType()` etc.

This patch fixes one callsite of  `getTargetAddressSpace` but there are several other existing callsites remaining which if called with a function, they *would not* return the appropriate address space.

If someone more familar with clang than me disagrees please chime in


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111566/new/

https://reviews.llvm.org/D111566