Sorry for the very late response to this!

Rebased and addressed some comments.

In D62574#1552220, @Anastasia wrote:

Ok, I think at some point you might want to test extra functionality that doesn't fit into OpenCL model, for example explicit conversion over non-overlapping address spaces? I think for this you might find useful to add the extra flag that switches on extra testing with "fake" setup of address spaces.

Replaced compatiblyIncludes and its dependents with ASTContext accessors instead.

I'll have to see if that's possible without breaking a few more interfaces, since you can throw around Qualifiers and check for compatibility without an ASTContext today.

I was just thinking about testing the new logic. Should we add something like -ffake-address-space-map (https://clang.llvm.org/docs/UsersManual.html#opencl-specific-options) that will force targets to use some implementation of fake address space conversions? It was quite useful in OpenCL before we added concrete targets with address spaces. Alternatively, we can try to use SPIR that is a generic Clang-only target that has address spaces.

Well, there are a couple targets which have target address spaces even today, I think. AMDGPU should be one, right?

Yes, however I am not sure we will be able to test more than what we test with OpenCL. Also I am not sure AMD maintainer would be ok. Do you have any concrete idea of what to test?

In D62574#1523835, @Anastasia wrote:

This patch does not address the issue with the accessors
on Qualifiers (isAddressSpaceSupersetOf, compatiblyIncludes),
because I don't know how to solve it without breaking a ton of
rather nice encapsulation. Either, every mention of compatiblyIncludes
must be replaced with a call to a corresponding ASTContext method,
Qualifiers must have a handle to ASTContext, or ASTContext must be
passed to every such call. This revision mentions the issue in a comment:
https://reviews.llvm.org/D49294

I think using ASTContext helper is ok then.

This was accepted a while ago, but never landed. I don't have commit access; could someone commit it?

In D58236#1467151, @Anastasia wrote:

In D58236#1466263, @ebevhan wrote:

Well, it doesn't seem to me like there is consensus on prohibiting nested address space conversion like this.

I can simply redo the patch to only include the bugfix on implicit conversions and drop the nesting level checks.

I thought the conclusion is:

• Explicit conversions allowed for nested pointers with a warning.
• Implicit conversions are disallowed for nested pointers with an error.
  
  Do you not see it this way?

Well, it doesn't seem to me like there is consensus on prohibiting nested address space conversion like this.

In D57464#1438213, @Anastasia wrote:

I think I would lean towards the latter since it means less fudging around with a whole bunch of unrelated methods. Do @rjmccall or @rsmith have any further opinions on this?

Ok, I can change the patch to prototype this approach. I might need some example test cases though.

In D58236#1443082, @rjmccall wrote:

I think C probably requires us to allow this under an explicit cast, but we can at least warn about it. It does not require us to allow this as an implicit conversion, and that should be an error.

Any more input on this?

In D57464#1425904, @Anastasia wrote:

I think not. :( But I am wondering if we could proceed for now in some general direction and then make any improvements later. Probably the biggest part of this patch is not going to change. Would this make sense?

Any more input on this?

In D58236#1412267, @Anastasia wrote:

LGTM! Thanks a lot for fixing this old bug! Btw, do you plan to look at generalizing this to C++ as well?

In D58060#1396565, @Anastasia wrote:

We had discussion related to this with John earlier. And I documented it in this bug: https://bugs.llvm.org/show_bug.cgi?id=39674

So static_cast permits conversions from AS1 to AS2 where that conversion is implicitly allowed, and the new addrspace_cast would permit conversions from AS1 to AS2 where it is explicitly allowed. That seems like it fits in rather well with the idea in D57464 regarding support for specifying permitted AS conversions in target.

In D57553#1381920, @leonardchan wrote:

In regards to solving the problem of resizing for int conversions, I'm starting to think that we will need that initial resize since if we want to retain the min-max pattern for all conversions, then it would be necessary to extend and shift when converting from an int to fixed point. Otherwise, I think we'd have the initial pattern I proposed where we check against the source value, but not have this pattern.

In D57464#1386614, @rjmccall wrote:

Moving parsed attributes between lists isn't unreasonable if that's what you have to do; we already do that when processing the ObjC ARC qualifiers. The ambiguity with function attributes is pretty much inherent.

This doesn't seem to address the particular case in the integer conversion patch. In fact, I don't see those conversions at all.

I think the code that comes to mind is mostly like in GetTypeSourceInfoForDeclarator:

LangAS AS =
    (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if it's not OpenCL++, but there shouldn't be a reason why the rest of the code in that block won't work for regular C++.

Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the patches when they're up. Haven't done much serious testing on my end so far, but from what I've seen the patches so far look good!

I'm a bit late to the party here, it was an older patch so I wasn't watching this one.

Nothing sticks out to me, so I think it looks good. Hard to tell if there are any sneaky edge cases in the lowering steps, though.

Thanks, that would be great! Hopefully it will work this time.

Use auto.

I'm also a bit confused about the semantics that this patch is applying to function types. It mostly seems to concern the extra trailing Qualifiers on CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, SemaDecl:3198) it seems to be applying the address space to the function type itself, which certainly seems like something else to me. A function with an address space qualified type would (to me, at least) be a function *located* in that address space, not one qualified to take a this from that address space.

Maybe a test with scale == bitwidth? Other than that it looks good to me.

Fix the build failures (caused by default argument promotion of float vectors).

Hmm, sorry about that. I'll have a look.

I don't have commit access, so I would appreciate it if you could commit the change.

Added testing for C++ and different sizes of int and long.

LGTM.

Rebased and addressed review comments.

In D53738#1310212, @leonardchan wrote:

In D53738#1309171, @ebevhan wrote:

In D53738#1308314, @leonardchan wrote:

Generally I think it's good! One final note; I assume we could technically reuse/rename the EmitFixedPointAdd function and use it to emit other binops when those are added?

Yes, but I imagine if we choose to keep the call to EmitFixedPointConversion to cast both operands to a common type, this wouldn't be reused for division or multiplication since I believe those do not require for the operands to be converted to a common type.

They don't? The example given by the spec is even int * _Fract.

Oh, I meant that the scales of both operands won't need to be aligned before performing the operation. Since for multiplication, we can multiply fixed point numbers in any scale without shifting and only need to perform a shift on the result to convert to the destination type. Although this would only apply to non-saturating multiplication since to use the intrinsics, both operands would need to be in the same scale.

In D53738#1308314, @leonardchan wrote:

Generally I think it's good! One final note; I assume we could technically reuse/rename the EmitFixedPointAdd function and use it to emit other binops when those are added?

Yes, but I imagine if we choose to keep the call to EmitFixedPointConversion to cast both operands to a common type, this wouldn't be reused for division or multiplication since I believe those do not require for the operands to be converted to a common type.

In D53738#1305498, @leonardchan wrote:

@ebevhan Any more comments on this patch?