[PATCH] D62574: Add support for target-configurable address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Sorry for the delay! I rebased the patch onto latest and it mostly worked, with a few hitches.



================
Comment at: clang/lib/Sema/SemaCast.cpp:2617-2619
+                  (Self.getLangOpts().OpenCL &&
+                   (DestPPointee->isFunctionType() ||
+                    SrcPPointee->isFunctionType())))) {
----------------
Without this, `SemaOpenCL/func.cl` fails since `foo((void*)foo)` is no longer an error. Since the function pointer is Default-qualified, `isExplicitAddrSpaceConversionLegal` delegates the check to the target, which will always return true.

To be honest, the clause in that method for letting targets allow conversions where `LangAS::Default` is involved only works so long as every type in OpenCL has the "right" semantic AS. When any of them are Default, things like this happen.

Maybe we should only ask the target about explicit conversions if both ASes are Default or a target AS?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:5289
 
+  // FIXME: hasAddressSpace is wrong; this check will be skipped if FromType is
+  // not qualified with an address space, but if there's no implicit conversion
----------------
Anastasia wrote:
> ebevhan wrote:
> > Anastasia wrote:
> > > Do you have a failing test case, if so feel free to create a bug?
> > Unsure how I'd make one. I suspect this can't be triggered in OpenCL++, because you can't really have LangAS::Default on FromType there, can you? It would always be some AS.
> > 
> > Doing it in another way would require a target that has configurable ASes, which doesn't exist yet. Also, it would require methods qualified with target ASes, and that doesn't work yet either.
> Ok, that's right in OpenCL almost every type gets an address space early in parsing.
> 
> But if we already know it's a bug and the fix for it we could change this code? Although I think the bug fix should better be done on a separate review.
I made the fix here just to try, and it caused a difference in address-space-lambda.clcpp.

I'm not sure why that lambda would be valid just because `mutable` is added... So this seems like the correct behavior to me?


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

https://reviews.llvm.org/D62574