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?

I'd be interested in seeing tests for two saturating unsigned _Fract with and without padding.

The only thing I didn't include in this patch were the suggested changes to FixedPointSemantics which would indicate if the semantics were original from an integer type. I'm not sure how necessary this is because AFAIK the only time we would need to care if the semantics came from an int is during conversion from a fixed point type to an int, which should only occur during casting and not binary operations. In that sense, I don't think this info needs to be something bound to the semantics, but more to the conversion operation. I also don't think that would be relevant for this patch since all integers get converted to fixed point types anyway and not the other way around.

In D53738#1288333, @rjmccall wrote:

In D53738#1287123, @ebevhan wrote:

In D53738#1284213, @rjmccall wrote:

Not out of line with other features that significantly break with what's expressible. But the easy alternative to storing the intermediate "type" in the AST is to just provide a global function that can compute it on demand.

Yeah, it might just be simpler to go the route of not storing the computation semantic in the AST, at least for now. That's fairly similar to the solution in the patch, so I feel a bit silly for just going around in a circle.

To make that work I think the patch needs some changes, though. There should be a function in FixedPointSemantics to find the common full-precision semantic between two semantics, and the getFixedPointSemantics in ASTContext should be amended to take integer types (or another method should be provided for this, but that one already exists). I think that the FixedPointConversions method should also be embedded into the rest of UsualArithmeticConversions as there shouldn't be any need to have it separate. You still want to do the rvalue conversion and other promotions, and the rules for fixed-point still fit into the arithmetic conversions, even in the spec.

EmitFixedPointConversion should be changed to take FixedPointSemantics rather than QualType. This has a couple of downsides:

• It can no longer handle floating point conversions. They would have to be handled in EmitScalarConversion.
• Conversion from integer is fine, but conversion to integer cannot be generalized away with the fixed-point semantics as they are currently, as that kind of conversion must round towards zero. This requires a rounding step for negative numbers before downscaling, which the current code does not do. Is there a better way of generalizing this?

FixedPointSemantics is free to do whatever is convenient for the representation; if it helps to make it able to explicitly represent an integral type — and then just assert that it's never in that form when it's used in certain places, I think that works. Although you might consider making a FixedPointOrIntegralSemantics class and then making FixedPointSemantics a subclass which adds nothing to the representation but semantically asserts that it's representing a fixed-point type.

In D53738#1284213, @rjmccall wrote:

2. The question is easily answered by pointing at the language spec. The language does not say that the operands are promoted to a common type; it says the result is determined numerically from the true numeric values of the operands.

I guess. I just see that as a behavioral specification and not necessarily an implementation detail. It's perfectly acceptable to implement it as promotion to a common type (obviously, as that's what we are going to do), and I don't really see this as the spec putting any kind of requirement on how the implementation should be done.

Of course it's a behavioral specification. All I'm saying is that the implementation detail of how we perform these operations isn't really reasonable or appropriate to express in the AST. I know it's a bit fuzzy because of some of the things we do with e.g. opaque values and so on, but the AST is not meant to be a completely implementation-defined IR; it tries to capture the formal semantics of the program including "official" conversions and so on. Integer addition is specified as converting its arguments to a common type and then performing a homogenous operation in that type. Fixed-point addition is not; it's specified as doing a heterogenous operation on the original (well, rvalue-converted) operand types.

In D53738#1283983, @rjmccall wrote:

In D53738#1283861, @ebevhan wrote:

In D53738#1283459, @rjmccall wrote:

Well, it could be passed around through most code as some sort of abstractly-represented intermediate "type" which could be either a QualType or a fixed-point semantics.

Sounds to me like you're describing a new Type that can contain an arbitrary fixed-point semantic :)

The fact that it doesn't exist in the modeled type system is important.

The arbitrary-type overflow intrinsics have this same problem, and we did not try to solve it by creating a new type class for arbitrary-precision integers and promoting the arguments.

In D53738#1283459, @rjmccall wrote:

Well, it could be passed around through most code as some sort of abstractly-represented intermediate "type" which could be either a QualType or a fixed-point semantics.

In D53738#1281332, @rjmccall wrote:

Well, maybe the cleanest solution would be to actually fold CompoundAssignOperator back into BinaryOperator and just allow BinaryOperator to optionally store information about the intermediate type of the computation and how the operands need to be promoted. That information could be elided in the common cases where it's trivial, of course.

In D53738#1279069, @rjmccall wrote:

The important difference would be that we separate the semantics of performing the conversion and the operation. I understand that adding a new type could be a bit more involved than baking the conversion into the operator, but I really do not enjoy seeing so much implicit, implementation-specific logic encapsulated in the same AST element. Anyone who wants to look at BinaryOperators with fixed-point types will need to know all of the details of how the finding the common type and conversion is done, rather than simply "it does an addition".

It's not just that adding a new type is "involved", it's that it's a bad solution. Those types can't actually be expressed in the language, and changing the type system to be able to express them is going to lead to a lot of pain elsewhere.

In D53738#1278078, @rjmccall wrote:

I don't think we should add *types* just for this, but if you need to make a different kind of BinaryOperator that represents that the semantics aren't quite the same (the same way that the compound assignments have their own subclass), that seems natural to me.

I think I've already expressed that I'm not at all a fan of encoding the full-precision logic in the operations themselves and not explicitly in the AST. We already have (well, not yet, but we will) routines for emitting conversions to/from/between fixed-point types of arbitrary semantics - why not use that instead of reimplementing the same logic for every binary operation?