[Fixed Point Arithmetic] Fixed Point Addition

[leonardchan]

Summary

This patch covers addition between fixed point types and other fixed point types or integers, using the conversion rules described in 4.1.4 of N1169.

Usual arithmetic rules do not apply to binary operations when one of the operands is a fixed point type, and the result of the operation must be calculated with the full precision of the operands, so we should not perform any casting to a common type.

This patch does not include constant expression evaluation for addition of fixed point types. That will be addressed in another patch since I think this one is already big enough.

[leonardchan]

clang/lib/CodeGen/CGExprScalar.cpp

3223–3247 ↗

(On Diff #171212)

There is a case when we cannot use the sadd/uadd intrinsics because those intrinsics saturate to the width of the operands when instead we want to saturate to the width of the resulting fixed point type. The problem is that if we are given operands with different scales that require extending then shifting before adding, the bit width no longer matches that of the resulting saturated type, so the sat intrinsics instead saturate to the extended width instead of the result type width. This could be a problem with my logic though and there could be a better way to implement addition.

Otherwise, as far as I can tell, this could be addressed by replacing the clamping with an intrinsic, which would create a use case for the signed/unsigned saturation intrinsics. Alternatively, the addition intrinsics could be expanded to also provide another argument that contains a desired saturation bit width, which would allow for collapsing the saturation type case into a call to this one intrinsic function.

[ebevhan]

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?

As it is now, EmitFixedPointAdd replicates the logic for both converting from integer to fixed-point and fixed-point to fixed-point. You mention a special case with saturating addition, but this special case only exists because the routine needs to do fixed-point->saturating fixed-point on its own. If all EmitFixedPointAdd did was perform an add between two fixed-point values with the same semantics, then the logic would be much simpler and the act of conversion would be delegated to the routines that actually handle it.

I want to float the idea again of adding an AST type to encapsulate an arbitrary fixed-point semantic and using that as the 'common type' for binary operations involving fixed-point types. This would enable UsualArithmeticConversions to handle fixed-point types similarly to how it does today (find the 'common' full precision type, implicitly cast the LHS and RHS if necessary). There is one new thing that it would have to do; the result after the operation should not be the full precision type, but rather be casted to the operand type of highest rank. I don't think the existing code in SemaExpr can handle the case of adding an implicit cast after the operation. I don't think it should be hard to add, though.

clang/lib/Sema/SemaExpr.cpp

1393 ↗

(On Diff #171212)

I'm not sure I understand why this is in a separate function. What part of this doesn't work in UsualArithmeticConversions, in a 'handleFixedPointConversion' similar to the other cases?

1416 ↗

(On Diff #171212)

I feel like this logic can be folded into the rank handling. Does it work properly if you give signed types higher rank than unsigned ones?

1435 ↗

(On Diff #171212)

This can be avoided by making all integer types lower rank than the fixed point types. The rank between them doesn't matter, either; they can all be the same.

[bjope]

clang/lib/CodeGen/CGExprScalar.cpp

3208 ↗

(On Diff #171212)

LHS = Builder.CreateIntCast(LHS, CommonTy, LHSSign);

(I think that it even is possible to remove the condition and always do this (the cast will be a no-op if LHSWidth==CommonWidth))

3211 ↗

(On Diff #171212)

Same as above, this can be simplified as:

RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);

3214 ↗

(On Diff #171212)

Arithmetically I think that it would be enough to align the scale to

std::max(std::min(LHSScale, RHSScale), ResultScale)

As adding low fractional bits outside the result precision, when we know that those bits are zero in either of the inputs, won't impact any more significant bits in the result.

Maybe your solution is easier and good enough for a first implementation. And maybe opt is smart enough to optimize this anyway. Or maybe codegen could be worse due to not using legal types. Or maybe codegen could be improved due to using sadd.sat in more cases?
Lots of "maybes", so I don't think you need to do anything about this right now. It is just an idea from my side.

3252 ↗

(On Diff #171212)

It should be possible to do it like this (if you add some line wrapping):

Builder.CreateBinaryIntrinsic(ResultSign ? llvm::Intrinsic::sadd_sat : 
 llvm::Intrinsic::uadd_sat, LHS, RHS);

[rjmccall]

In D53738#1277172, @ebevhan wrote:

I want to float the idea again of adding an AST type to encapsulate an arbitrary fixed-point semantic and using that as the 'common type' for binary operations involving fixed-point types. This would enable UsualArithmeticConversions to handle fixed-point types similarly to how it does today (find the 'common' full precision type, implicitly cast the LHS and RHS if necessary). There is one new thing that it would have to do; the result after the operation should not be the full precision type, but rather be casted to the operand type of highest rank. I don't think the existing code in SemaExpr can handle the case of adding an implicit cast after the operation. I don't think it should be hard to add, though.

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.

[ebevhan]

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 don't know if adding a new operator kind was what I had in mind. The operator hasn't changed, it's still a normal binary operator. Compound assignments are their own operator with its own syntax; it doesn't really strike me as the same thing.

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".

[rjmccall]

In D53738#1278528, @ebevhan wrote:

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 don't know if adding a new operator kind was what I had in mind. The operator hasn't changed, it's still a normal binary operator. Compound assignments are their own operator with its own syntax; it doesn't really strike me as the same thing.

Sorry for being unclear. I wasn't suggesting adding a new BinaryOperatorKind, I was suggesting making a subclass of BinaryOperator that stored the extra information you'd like to store.

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.

[ebevhan]

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.

I did figure that it might be a bit of work to adapt other parts of the code to handle the new type, but I just prefer separating the concerns and being explicit about what work is performed. To me, the clearest way of doing that is by handling the conversions explicitly in the AST, and separately from the operators themselves. Also, not being able to deal in QualTypes for the common full precision type handling means that reusing the operator handling code in Sema won't be easy, or even possible. It would have to be computed in CreateBuiltinBinOp, since it's not possible to forward anything but QualType from the CheckXXXOperands functions.

If you don't think it's a good idea I'll concede, but then there's the question of how to get the full precision semantics into the operator (if we store it there). I guess the most straightforward path is something like:

• In CreateBuiltinBinOp, do the normal Check function to get the result type
• If the result is a fixed-point type, go into a separate code path
• Ask a method for the common FixedPointSemantics of the given LHS and RHS
• Build the correct BinaryOperator subclass.

I need to think about this to see if our downstream implementation can be adapted to work with this design.

Compound assignments are already their own subclass, though. Unless the full precision semantic information is simply baked into the regular BinaryOperator, it would require two new subclasses: one for normal full precision operators and one for compound ones? Wouldn't adding this require new hooks and code paths in visitors, even though there's really nothing different about the operator?

The type information that CompoundAssignOperator stores today is rather similar to what a full precision operator would need, though it would need to store a FixedPointSemantics instead.

---

I do have more comments on the code at the moment, but I'm holding off a bit on the review while we iron out some of these details.

clang/lib/CodeGen/CGExprScalar.cpp

3156 ↗

(On Diff #171212)

This function should not be necessary. Instead, add to FixedPointSemantics:

• static FixedPointSemantics GetIntegerSemantics(unsigned Width, bool IsSigned) to get an FPS for a specific integer width and signedness (width=Width, scale=0, sat=false, signed=IsSigned, padding=false)
• FixedPointSemantics getCommonSemantics(const FixedPointSemantics &Other) to get an FPS for the common full precision semantic between this FPS and another one

3197 ↗

(On Diff #171212)

I think all of this (or at least something equivalent to it) should be calculated in Sema, not in CodeGen.

If we go with the idea of storing the full precision semantics in the operator, all the code in CodeGen should have to do is call EmitFixedPointConversion on the LHS and RHS with the FixedPointSemantics given by the operator. Same for converting back after the operation is performed.

clang/lib/Sema/SemaExpr.cpp

1310 ↗

(On Diff #171212)

Maybe this should be a method on ASTContext itself? It's probably useful in other cases.

9219 ↗

(On Diff #171212)

I think this 'commutativity' should be handled inside the function rather than here.

[rjmccall]

In D53738#1280193, @ebevhan wrote:

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.

I did figure that it might be a bit of work to adapt other parts of the code to handle the new type, but I just prefer separating the concerns and being explicit about what work is performed. To me, the clearest way of doing that is by handling the conversions explicitly in the AST, and separately from the operators themselves. Also, not being able to deal in QualTypes for the common full precision type handling means that reusing the operator handling code in Sema won't be easy, or even possible. It would have to be computed in CreateBuiltinBinOp, since it's not possible to forward anything but QualType from the CheckXXXOperands functions.

If you don't think it's a good idea I'll concede, but then there's the question of how to get the full precision semantics into the operator (if we store it there). I guess the most straightforward path is something like:

• In CreateBuiltinBinOp, do the normal Check function to get the result type
• If the result is a fixed-point type, go into a separate code path
• Ask a method for the common FixedPointSemantics of the given LHS and RHS
• Build the correct BinaryOperator subclass.

I need to think about this to see if our downstream implementation can be adapted to work with this design.

Compound assignments are already their own subclass, though. Unless the full precision semantic information is simply baked into the regular BinaryOperator, it would require two new subclasses: one for normal full precision operators and one for compound ones? Wouldn't adding this require new hooks and code paths in visitors, even though there's really nothing different about the operator?

The type information that CompoundAssignOperator stores today is rather similar to what a full precision operator would need, though it would need to store a FixedPointSemantics instead.

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.

The infinite-precision rule here is still internal to an individual operator, right? The standard's not trying to say that we should evaluate x + y < z by doing a comparison as if all the operands were individually infinite-precision?

[ebevhan]

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.

That sounds like a fairly hefty refactor. Also, doing the fold would put the extra QualType info from CAO into BO, sure, but this cannot work for the full-precision case since we can't represent those with QualTypes. The information for the full precision 'type' would have to be stored separately anyway.

Or did you mean to make a subclass of that new BinaryOperator for the full precision case, and store the full precision info there?

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

The infinite-precision rule here is still internal to an individual operator, right? The standard's not trying to say that we should evaluate x + y < z by doing a comparison as if all the operands were individually infinite-precision?

Correct, the result of the computation is 'implicitly converted' back to the result type after the operation is performed. The type of the expression will always be the result type, never the full precision type.

As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.

[rjmccall]

In D53738#1281894, @ebevhan wrote:

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.

That sounds like a fairly hefty refactor. Also, doing the fold would put the extra QualType info from CAO into BO, sure, but this cannot work for the full-precision case since we can't represent those with QualTypes. The information for the full precision 'type' would have to be stored separately anyway.

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.

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

The infinite-precision rule here is still internal to an individual operator, right? The standard's not trying to say that we should evaluate x + y < z by doing a comparison as if all the operands were individually infinite-precision?

Correct, the result of the computation is 'implicitly converted' back to the result type after the operation is performed. The type of the expression will always be the result type, never the full precision type.

Okay, good.

As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.

...bitwise?

[ebevhan]

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 :)

It still feels like this just introduces inconsistencies into the form of the AST. If we do add this extra type object to the BO, won't people wonder why we use ImplicitCastExpr for non-fixedpoint operations but use the special QualTypeOrFPSemantics BinaryOperator::ComputationType; for fixedpoint operations even though they both have nearly the same semantic meaning (converting to a common type before doing the operation)?

(The difference being that using the ComputationType requires you to cast back to the result type afterwards.)

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.

...bitwise?

The spec uses different wording for the arithmetic operations and comparison operations, and it's not entirely obvious what it means. For the arithmetic operators, it has the whole section on finding the full precision common type, but for comparisons it says:

When comparing fixed-point values with fixed-point values or integer values,
the values are compared directly; the values of the operands are not converted before the
comparison is made.

What 'directly' means in conjunction with 'the operands are not converted' is not clear. It's reasonable to assume that it either means comparing value-wise (so, the same as finding a common type that fits all values and then comparing; though this would obviously require conversion), or perhaps 'directly' means a bitwise (representation) comparison. The latter seems a bit farfetched to me, though.

[rjmccall]

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.

It still feels like this just introduces inconsistencies into the form of the AST. If we do add this extra type object to the BO, won't people wonder why we use ImplicitCastExpr for non-fixedpoint operations but use the special QualTypeOrFPSemantics BinaryOperator::ComputationType; for fixedpoint operations even though they both have nearly the same semantic meaning (converting to a common type before doing the operation)?

1. You would have an inconsistency in either case, since e.g. numeric + otherwise always returns the same type as its operands, but this would not.
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.

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

Take a look at how DeclRefExpr stores its various optional components.

As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.

...bitwise?

The spec uses different wording for the arithmetic operations and comparison operations, and it's not entirely obvious what it means. For the arithmetic operators, it has the whole section on finding the full precision common type, but for comparisons it says:

When comparing fixed-point values with fixed-point values or integer values,
the values are compared directly; the values of the operands are not converted before the
comparison is made.

What 'directly' means in conjunction with 'the operands are not converted' is not clear. It's reasonable to assume that it either means comparing value-wise (so, the same as finding a common type that fits all values and then comparing; though this would obviously require conversion), or perhaps 'directly' means a bitwise (representation) comparison. The latter seems a bit farfetched to me, though.

I think this is just like fixed-point arithmetic: you should do an exact numeric comparison, but there's no formal conversion because it would have to be a conversion to some concrete type, which could leave to (mandatory!) inexactness when there's no common type that expresses the full range of both operands.

Arguably this is all how integer arithmetic and comparisons should work, but that's not something they can fix in a TR. (Floating-point doesn't need to work this way because the FP types subset each other.)

[ebevhan]

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.

Okay, I had a look at how those were emitted, I wasn't familiar with it until now. That's certainly a way of approaching this implementation as well, though I think the full precision info should still be in the AST somewhere rather than implied from the operand types until CodeGen.

It still feels like this just introduces inconsistencies into the form of the AST. If we do add this extra type object to the BO, won't people wonder why we use ImplicitCastExpr for non-fixedpoint operations but use the special QualTypeOrFPSemantics BinaryOperator::ComputationType; for fixedpoint operations even though they both have nearly the same semantic meaning (converting to a common type before doing the operation)?

1. You would have an inconsistency in either case, since e.g. numeric + otherwise always returns the same type as its operands, but this would not.

True, but the CompoundAssignOperator already has that inconsistency with ComputationResultType.

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.

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

Take a look at how DeclRefExpr stores its various optional components.

TrailingObjects, then. That certainly wouldn't work if CompoundAssignOperator is still a subclass of BinaryOperator, so it would have to be folded in that case.

So the implementation would be with a TrailingObjects<BinaryOperator, QualType, FixedPointSemantics> where it can have 2 QualTypes and 1 FixedPointSemantics, the QualTypes being subsumed from CompoundAssignOperator.

Probably still quite a hefty amount of code that would have to be updated to make this change.

As a side note, comparisons are still a bit up in the air. I don't think we came to a conclusion on whether they should be done in full precision or bitwise. The spec isn't clear.

...bitwise?

The spec uses different wording for the arithmetic operations and comparison operations, and it's not entirely obvious what it means. For the arithmetic operators, it has the whole section on finding the full precision common type, but for comparisons it says:

When comparing fixed-point values with fixed-point values or integer values,
the values are compared directly; the values of the operands are not converted before the
comparison is made.

What 'directly' means in conjunction with 'the operands are not converted' is not clear. It's reasonable to assume that it either means comparing value-wise (so, the same as finding a common type that fits all values and then comparing; though this would obviously require conversion), or perhaps 'directly' means a bitwise (representation) comparison. The latter seems a bit farfetched to me, though.

I think this is just like fixed-point arithmetic: you should do an exact numeric comparison, but there's no formal conversion because it would have to be a conversion to some concrete type, which could leave to (mandatory!) inexactness when there's no common type that expresses the full range of both operands.

This is probably the correct interpretation, I just think it could have been stated a bit clearer that they pretty much do mean the same thing for the comparison operators as for the arithmetic operators.

[rjmccall]

1. You would have an inconsistency in either case, since e.g. numeric + otherwise always returns the same type as its operands, but this would not.

True, but the CompoundAssignOperator already has that inconsistency with ComputationResultType.

Right, but it's the same inconsistency, which is why I'm suggesting that you treat this as an opportunity to generalize it. Or we can avoid recording it and just make it up in IRGen.

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.

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

Take a look at how DeclRefExpr stores its various optional components.

TrailingObjects, then. That certainly wouldn't work if CompoundAssignOperator is still a subclass of BinaryOperator, so it would have to be folded in that case.

So the implementation would be with a TrailingObjects<BinaryOperator, QualType, FixedPointSemantics> where it can have 2 QualTypes and 1 FixedPointSemantics, the QualTypes being subsumed from CompoundAssignOperator.

Probably still quite a hefty amount of code that would have to be updated to make this change.

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.

[ebevhan]

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.

Okay, those are good points. I guess I might have been a bit too focused on reusing the behavior of the AST to fit the implementation, but that doesn't seem to be the intention with it.

It might just be easier to store the full-precision info in BO directly. BO might be too common to warrant the size increase, though. FixedPointSemantics can probably be optimized to only take 32 bits.

What you can definitely do is store a bit in BO saying that there's extra storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

Take a look at how DeclRefExpr stores its various optional components.

TrailingObjects, then. That certainly wouldn't work if CompoundAssignOperator is still a subclass of BinaryOperator, so it would have to be folded in that case.

So the implementation would be with a TrailingObjects<BinaryOperator, QualType, FixedPointSemantics> where it can have 2 QualTypes and 1 FixedPointSemantics, the QualTypes being subsumed from CompoundAssignOperator.

Probably still quite a hefty amount of code that would have to be updated to make this change.

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?

All EmitFixedPointAdd should have to do is get the semantics of the operands and result type, get their full-precision semantic, call EmitFixedPointConversion on both operands, do the addition, and call it again to convert back to the result value. Move as much of the conversions as possible out of the function.

Does all this sound reasonable?

[rjmccall]

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.

All EmitFixedPointAdd should have to do is get the semantics of the operands and result type, get their full-precision semantic, call EmitFixedPointConversion on both operands, do the addition, and call it again to convert back to the result value. Move as much of the conversions as possible out of the function.

Does all this sound reasonable?

Makes sense to me.

[ebevhan]

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.

It might just be simpler and a bit more general to add a DownscalingRoundsTowardZero field to FixedPointSemantics, which specifies that the source value should be rounded toward zero before downscaling. Then conversion routines could handle the integer case explicitly. I'm not sure if the field would be useful for anything else, though; it has a pretty specific meaning.

I think it's a bit odd to have FixedPointSemantics as a specialization of something else, since technically integers are a specialization of fixed-point values and not the other way around.

[rjmccall]

In D53738#1288372, @ebevhan wrote:

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.

It might just be simpler and a bit more general to add a DownscalingRoundsTowardZero field to FixedPointSemantics, which specifies that the source value should be rounded toward zero before downscaling. Then conversion routines could handle the integer case explicitly. I'm not sure if the field would be useful for anything else, though; it has a pretty specific meaning.

I think it's a bit odd to have FixedPointSemantics as a specialization of something else, since technically integers are a specialization of fixed-point values and not the other way around.

Well, it's not a specialization of "integer", it's a specialization of "fixed-point or integer". It depends on how useful it ends up being to know that something statically isn't integral.

[leonardchan]

Sorry for the delay in responding to this. Been working on a few other patches.

A couple of the bigger changes so far:

• Added getCommonSemantics to FixedPointSemantics for finding a common full precision semantic.
• getFixedPointSemantics accepts an int type.
• FixedPointConversions was renamed to handleFixedPointConversions and is a part of UsualArithmeticConversions
• EmitFixedPointAdd now works by converting both operands to a common FixedPointSemantics that can hold the full precision of the result, performing a regular add or call to the addition intrinsics, and converting back to the result type if necessary.
  • This also fixed the case with calling the sat add intrinsics.
• Change EmitFixedPointConversion to accept FixedPointSemantics. I kept the original one still which takes QualTypes that are fixed point or integer types and gets the FixedPointSemantics from them.

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.

clang/lib/CodeGen/CGExprScalar.cpp

3208 ↗

(On Diff #171212)

Replaced this part with converting the operands to common semantics, then regular addition.

3211 ↗

(On Diff #171212)

Same as above.

3214 ↗

(On Diff #171212)

Same as above.

3223–3247 ↗

(On Diff #171212)

Fixed by converting operands to common fixed point semantics, adding, then converting the result. This ends up producing the same instructions as the original tests.

clang/lib/Sema/SemaExpr.cpp

1393 ↗

(On Diff #171212)

I might've misinterpreted the standard in that operations between fixed point and other fixed point or integers didn't fall under usual arithmetic conversions, but I can see how it would be simpler to keep in in UAC since almost all checks on binary operations call this.

Added a changed FixedPointConversions to handleFixedPointConversion.

1416 ↗

(On Diff #171212)

I think this is necessary since it seems the purpose of this is to align the scales between signed and unsigned types if they are different. For signed and unsigned types specifically, the standard also says unsigned fixed-point operand is converted to its corresponding signed fixed-point type which I think means it's an actual conversion.

[leonardchan]

For the integer conversion though, I was going to add that in a separate fixed-point-to-int and int-to-fixed-point patch.

[ebevhan]

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.

For the integer conversion though, I was going to add that in a separate fixed-point-to-int and int-to-fixed-point patch.

Okay, that's fine! You're right in that the semantics commoning will never produce an integer semantic like that.

clang/lib/Basic/FixedPoint.cpp

129 ↗

(On Diff #173961)

Does this properly compensate for types of equal width but different signedness?

If you have a signed 8-bit 7-scale fixed point number, and operate with an unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB at the same time. I think you need an extra bit.

135 ↗

(On Diff #173961)

If one has padding but the other doesn't, then the padding must be significant, so the full precision semantic cannot have padding.

clang/lib/CodeGen/CGExprScalar.cpp

3385 ↗

(On Diff #173961)

Can EmitFixedPointConversion not determine this on its own?

3387 ↗

(On Diff #173961)

'align' usually means something else. Maybe 'Convert the operands to the full precision type' and 'FullLHS'?

clang/lib/Sema/SemaExpr.cpp

1306 ↗

(On Diff #173961)

Does this handle compound assignment? The other functions handle this specially.

1403 ↗

(On Diff #173961)

Can this commutation be folded into the function to align it with the rest?

[leonardchan]

clang/lib/Basic/FixedPoint.cpp

129 ↗

(On Diff #173961)

Added test for this also.

clang/lib/Sema/SemaExpr.cpp

1306 ↗

(On Diff #173961)

Currently no. I was initially intending to add addition compound assignment in a different patch, but do you think it would be appropriate to include it here? I imagine the changes to Sema wouldn't be difficult, but would probably require adding a lot more tests.

[ebevhan]

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

If the padding case emits a uadd_sat, that seems wrong; uadd_sat doesn't saturate on the padding bit, but will saturate the whole number, which can result in invalid representation both with or without saturation taking effect.

Common semantics for unsigned types with padding might need a bit of trickery to get it to do the right thing.

clang/lib/CodeGen/CGExprScalar.cpp

3383 ↗

(On Diff #174080)

Missing a C there.

clang/lib/Sema/SemaExpr.cpp

1306 ↗

(On Diff #173961)

That's fine by me, then. Take it in the next patch.

clang/test/Frontend/fixed_point_add.c

36 ↗

(On Diff #174080)

What do these comments refer to? The scale is the same for both operands here.

[leonardchan]

In D53738#1299557, @ebevhan wrote:

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

If the padding case emits a uadd_sat, that seems wrong; uadd_sat doesn't saturate on the padding bit, but will saturate the whole number, which can result in invalid representation both with or without saturation taking effect.

Common semantics for unsigned types with padding might need a bit of trickery to get it to do the right thing.

Good case to bring up. For addition, I think we just need to add an extra condition that checks for unsigned padding in the result. Added this test also.

clang/test/Frontend/fixed_point_add.c

36 ↗

(On Diff #174080)

This was meant to be a addition with a short _Accum and _Fract, but accidentally typed this instead. Added that test after this one.

[leonardchan]

Good case to bring up. For addition, I think we just need to add an extra condition that checks for unsigned padding in the result. Added this test also.

Actually this was wrong. Updated and instead dictate how the appropriate number of bits to getCommonSemantics. The saturation intrinsics handle unsigned padding types correctly now and only add the extra padding bit to the common width if both operands have unsigned padding and the result is not saturated (to prevent an unnecessary ext + trunc on non saturating operations).

[ebevhan]

clang/lib/CodeGen/CGExprScalar.cpp

3385 ↗

(On Diff #173961)

What I meant here was that rather than zeroing out the saturation mode on the common semantic because we know that the common conversion won't saturate, EmitFixedPointConversion should be able to pick up on the fact that converting from semantic A to semantic B shouldn't cause saturation and not emit a saturating conversion in that case. Then you can set the saturation on the common semantic properly, since the operation should be saturating.

Even for something like sat_uf * sat_uf with padding, where the common type conversion should be (16w, 15scale, uns, sat, pad) -> (15w, 15scale, uns, sat, nopad), EmitFixedPointConversion shouldn't emit a saturation, since the number of integral bits hasn't changed.

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

This is probably a candidate for an isel optimization. This operation also works as an i16 ssat.add with a negative-clamp-to-zero afterwards, and if the target supports i16 ssat.add natively then it will likely be a lot more efficient than whatever an i15 uadd.sat produces.

[leonardchan]

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

Do you think it would be more efficient for now then if instead we did SHL by 1, saturate, then [AL]SHR by 1? This way we could use i16 ssat.add instead of i15 ssat.add?

[leonardchan]

@ebevhan Any more comments on this patch?

[ebevhan]

In D53738#1305498, @leonardchan wrote:

@ebevhan Any more comments on this patch?

It's strange, I feel like I've responded to the last comment twice now but there's nothing in Phab.

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?

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

We should probably just do it in isel or instcombine instead. We don't know at this point which intrinsic is a better choice (though, I think power-of-two-types are generally better).

[leonardchan]

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.

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

Ok. Are you suggesting something should be changed here though? I imagine that during legalization, i15 ssat.add would be legalized into i16 ssat.add if that is what's natively supported.

[ebevhan]

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.

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

No, it doesn't have to be changed. Just something to keep in mind.

i15 ssat.add would be legalized into i16 ssat.add if that is what's natively supported.

Sure, but I meant that i15 usat.add could be more efficient as i16 ssat.add.

[leonardchan]

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.

@rjmccall Any more comments on this patch?

clang/test/Frontend/fixed_point_add.c

269 ↗

(On Diff #174279)

Got it. Thanks!

[ebevhan]

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.

I see what you mean, but the fixed-point multiplication intrinsic requires the operands to be in the same scale.

It's certainly interesting to degenerate integer-with-fixedpoint to just a mul (since the scaling factor is 2^-n * 2^0, which is just 2^-n), but in the general case you can't avoid doing the scale alignment. Unless I'm missing something.

[leonardchan]

It's certainly interesting to degenerate integer-with-fixedpoint to just a mul (since the scaling factor is 2^-n * 2^0, which is just 2^-n), but in the general case you can't avoid doing the scale alignment. Unless I'm missing something.

No you're right. Sorry, for some reason I kept only thinking about the saturation fixed point intrinsics and not the regular fixed point ones.

@rjmccall Any more comments on this patch? As soon as I get LGTM, I can reuse the functions here for subtraction and use the updated UsualArithmeticConversions for casing between fixed point types in other patches.

[rjmccall]

clang/include/clang/AST/ASTContext.h

2638 ↗

(On Diff #174640)

Please include in the comment here that, unlike getCorrespondingUnsignedType, this has to be specific to fixed-point types because there are unsigned integer types like bool and char8_t that don't have signed equivalents.

clang/include/clang/Basic/FixedPoint.h

41 ↗

(On Diff #174640)

assert(!(IsSigned && HasUnsignedPadding) && ...);, please.

67 ↗

(On Diff #174640)

Actually representing the fully-precise value is operation-specific; you'd need a different calculation for at least addition and multiplication, and maybe also subtraction and (maybe?) division. Is this what you actually want?

70 ↗

(On Diff #174640)

This is trivial enough that it should probably be implemented inline.

clang/lib/AST/ASTContext.cpp

10489 ↗

(On Diff #174640)

Does getIntWidth do the thing you want for bool?

clang/lib/Basic/FixedPoint.cpp

141 ↗

(On Diff #174640)

Is this right? If I have a+b+c+d+e+f+g+h+i, where those are all the same signed type, won't the width of the common semantics keep growing as I add more operands?

[leonardchan]

clang/include/clang/Basic/FixedPoint.h

67 ↗

(On Diff #174640)

For addition and subtraction, I believe we just need to extend and shift to a type that is large enough to hold the larger scale and integral bits, then we can do a normal add/sub. I think the relational operations can use this also since we would just need to resize and align scales for them.

For multiplication, the intrinsics we will use also assume the scale for both operands are the same, which I believe will also require finding semantics large enough to hold the larger scale and integral bits.

declare iN @llvm.smul.fix(iN %L, %iN R, i32 %Scale)

Division is similar since the saturating and non-saturating intrinsics assume both operands have the same scale.

declare iN @llvm.sdiv.fix(iN %L, %iN R, i32 %Scale)

In each of these cases, I believe the common semantics just needs to be large enough to hold the scale and largest representable value, which is what this method does.

clang/lib/AST/ASTContext.cpp

10489 ↗

(On Diff #174640)

I think so. The resulting semantics for this should be unsigned with a width of 1. I believe a _Bool currently never gets passed to this since UsualUnaryConversions casts a _Bool into an int.

Currently adding a fixed point type with a bool is treated the same as adding with an int. Added a test for this also.

clang/lib/Basic/FixedPoint.cpp

141 ↗

(On Diff #174640)

This should be right. Each addition is split into separate binary operations which produce separate common fixed point semantics from 2 different semantics. The width is also primarily made from the larger scale and large number of integral bits (which excludes the sign or unsigned padding).

For example:

a+b -> Width: 32 (scale 14 + integral bits 16 + sign bit 1)
(a+b)+c -> Width: 32 ^^^^^
((a+b)+c)+d -> Width: 32 ^^^^^

Added a test for this.

[rjmccall]

clang/include/clang/Basic/FixedPoint.h

67 ↗

(On Diff #174640)

Okay, so I believe what you're saying is that getCommonSemantics is defined as returning a semantics that is capable of precisely representing both input values, not one that's capable of precisely representing the result of the computation. The documentation could be clearer about that.

clang/lib/Basic/FixedPoint.cpp

141 ↗

(On Diff #174640)

I see. If that's done consistently then it's fine that it technically slightly misrepresents the range of negative values.

clang/lib/CodeGen/CGExprScalar.cpp

3370 ↗

(On Diff #175965)

Please use cast.

clang/lib/Sema/SemaExpr.cpp

1237 ↗

(On Diff #175965)

Please optimize this function around expecting a BuiltinType. That is, instead of separately checking for an integer type here, please assert it when (1) the getAs fails or (2) you fall into the default case.

1304 ↗

(On Diff #175965)

Hmm. So adding a signed integer to an unsigned fixed-point type always converts the integer to unsigned? That's a little weird, but only because the fixed-point rule seems to be the other way. Anyway, I assume it's not a bug in the spec.

[ebevhan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

handleFixedPointConversion only calculates the result type of the expression, not the calculation type. The final result type of the operation will be the unsigned fixed-point type, but the calculation itself will be done in a signed type with enough precision to represent both the signed integer and the unsigned fixed-point type.

Though, if the result ends up being negative, the final result is undefined unless the type is saturating. I don't think there is a test for the saturating case (signed integer + unsigned saturating fixed-point) in the SaturatedAddition tests.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

handleFixedPointConversion only calculates the result type of the expression, not the calculation type.

Right, I understand that, but the result type of the expression obviously impacts the expressive range of the result, since you can end up with a negative value.

Hmm. With that said, if the general principle is to perform the operation with full precision on the original operand values, why are unsigned fixed-point operands coerced to their corresponding signed types *before* the operation? This is precision-limiting if the unsigned representation doesn't use a padding bit. That seems like a bug in the spec.

[leonardchan]

clang/include/clang/Basic/FixedPoint.h

67 ↗

(On Diff #174640)

Yup. Updated the documentation.

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Hmm. With that said, if the general principle is to perform the operation with full precision on the original operand values, why are unsigned fixed-point operands coerced to their corresponding signed types *before* the operation? This is precision-limiting if the unsigned representation doesn't use a padding bit. That seems like a bug in the spec.

Possibly. When the standard mentions converting from signed to unsigned fixed point, the only requirement involved is conservation of magnitude (the number of integral bits excluding the sign)

when signed and unsigned fixed-point types are mixed, the unsigned type is converted to the corresponding signed type, and this should go without loss of magnitude

This is just speculation, but I'm under the impression that not as much "attention" was given for unsigned types as for signed types since "In the embedded processor world, support for unsigned fixed-point data types is rare; normally only signed fixed-point data types are supported", but was kept anyway since unsigned types are used a lot.

However, to disallow unsigned fixed-point arithmetic from programming languages (in general, and from C in particular) based on this observation, seems overly restrictive.

I also imagine that if the programmer needs more precision, the correct approach would be to cast up to a type with a higher scale. The standard seems to make an effort to expose as much in regards to the underlying fixed point types as possible:

it should be possible to write fixed-point algorithms that are independent of the actual fixed-point hardware support. This implies that a programmer (or a running program) should have access to all parameters that define the behavior of the underlying hardware (in other words: even if these parameters are implementation-defined).

So the user would at least know that unsigned types may not have the same scale as their corresponding signed types if the hardware handles them with different scales.

Also added test for signed integer + unsigned saturating fixed point.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

As long as we maintain the same typing behavior, does the standard permit us to just Do The Right Thing here and do the extended arithmetic with the original unsigned operand? I'm sure there are some cases where we would produce a slightly different value than an implementation that does the coercion before the operation, but that might be permitted under the standard, and as you say, it would only affect some situations that it doesn't seem the standard has given much thought to.

[ebevhan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

The coercion of unsigned to signed is likely done for pragmatic reasons; if you have a signed and unsigned type of the same rank, operating on them together won't require any 'extra bits'. This means that if your hardware has native support for the operations, you won't end up in a situation where you really need 33 or 34 bits (for a 32 bit value) to actually perform the operation. This is one of the benefits of these kinds of implicit conversions.

It probably makes the implementation a bit easier as well, since you don't have to consider cases where both sides have different signedness. The loss of the extra bit of precision from the unsigned operand is probably not considered to be very important. As Leonard said, if you wanted higher precision you can simply use the next rank of types. ...Of course, they seem to have forgotten that that exact issue with differing signedness also applies to operations with fixed-point and integer operands.

Personally, I think the spec could be a bit more restrictive with how it handles these conversions, as the way they are now makes it really hard to generate code sensibly for machines that have native support for these kinds of operations; in many cases you'll end up requiring a couple bits too many or a couple bits too few to fit in a sensible register size, and ensuring that the scales match what your hardware can handle is also a challenge.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

The coercion of unsigned to signed is likely done for pragmatic reasons; if you have a signed and unsigned type of the same rank, operating on them together won't require any 'extra bits'. This means that if your hardware has native support for the operations, you won't end up in a situation where you really need 33 or 34 bits (for a 32 bit value) to actually perform the operation. This is one of the benefits of these kinds of implicit conversions.

Yeah, I can see that. I think it really depends on the target. If you have hardware support, and you're using an unpadded representation for unsigned fixed-point values, then preserving the extra bit on mixed-signedness operations will sometimes prevent you from using the hardware optimally. On the other hand, if you don't have hardware support, preserving the extra bit is usually easier than not: e.g., IIUC, multiplying a 32-bit signed _Fract with a 32-bit unpadded unsigned _Fract means just doing a 64-bit multiplication and dropping the low 32 bits, whereas doing the conversion first actually requires extra operations to ensure that the low bit doesn't contribute to the result. And a target with hardware support is probably going to use a padded format for unsigned fixed-point precisely so that it can take advantage of the hardware.

...Of course, they seem to have forgotten that that exact issue with differing signedness also applies to operations with fixed-point and integer operands.

Right.

[leonardchan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

For signed-unsigned multiplication, we could do that without having to do the conversion first, but in the case of addition and subtraction, we would still need to drop a bit with unpadded unsigned types so both have the same scales.

I don't want to make too many assumptions about hardware support, but I imagine that if one supports 32 bit signed addition, and unsigned types did not have padding, it would be better instead to LSHR on unsigned fixed point type then do 32 bit signed addition rather than SEXT+SHL on the signed fixed point type then do 33 bit signed addition.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

[I}n the case of addition and subtraction, we would still need to drop a bit with unpadded unsigned types so both have the same scales.

Right, but I don't think it matters because you're dropping that bit either way: with the specified semantics, you do the conversion beforehand which drops the bit, and with full-precision semantics, the bit isn't going to be representable and dropping it during the computation is equivalent to either rounding down (addition) or up (subtraction), either of which is allowed.

I don't want to make too many assumptions about hardware support, but I imagine that if one supports 32 bit signed addition, and unsigned types did not have padding, it would be better instead to LSHR on unsigned fixed point type then do 32 bit signed addition rather than SEXT+SHL on the signed fixed point type then do 33 bit signed addition.

My point is just that (1) the 33-bit signed addition can just be a 32-bit signed addition given that you ultimately have to produce a 32-bit result and (2) targets with hardware support are unlikely to use an unpadded unsigned type anyway. Although I suppose (2) is a bit questionable because hardware support could be added well after an ABI has been settled on.

[leonardchan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Ok. So should anything be changed here then if both semantics work? Currently we still do the 32 bit addition for signed and unsigned types.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Well, most of what we're doing here is trying to lay good groundwork for all operations, and "both semantics work" is only true for same-rank addition, which is why I'm asking whether we should consider what semantics we actually want in general.

[leonardchan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

I think the semantics for all binary arithmetic operators (+, -, *, /) can be simplified to:

1. If one operand is a signed fixed point type and the other an unsigned type, convert the unsigned type to a signed type (which may involve reducing the scale).
2. Find the common type of both operands that can hold both the larger scale and magnitude of the 2 operands, and cast them to this common type.
3. Perform the binary operation.
4. Cast to result type.

I think the semantics for addition are already settled with these semantics, and I believe subtraction shares the same semantics as those for addition. As you said before, the extra bit would not even matter with full precision since it would be trimmed anyway when converting from the common type to the result, and rounding can be either up or down.

Signed integer addition/subtraction with an unsigned fixed point type with these semantics will only lead to undefined behavior if the difference in the values between both types is negative since a negative value cannot be represented by an unsigned fixed point type (unless the result is saturating).

For multiplication and division, the result could technically be calculated also without having to cast between both sides, although for using the fixed point mul/div intrinsics, both operands would need to be casted to a common type with the same scale anyway. If a bit does end up getting dropped when casting an operand from unsigned to signed in (1), that dropped bit would not be representable when casting to the result anyway and the standard still allows us to return a rounded result.

When multiplying or dividing between a fixed point and integer though, we could just perform a regular integer mul/div if the result is non-saturating without having to perform the steps above. For saturating mul/div with an int, it might be easier to cast both sides to a common type then call the saturating fixed point mul/div intrinsics. In either case, I do not think rounding will be an issue here since we can still calculate the result precisely and the result will be either rounded up or down.

These semantics do not apply to comparison/relational operands, where if we do compare operands of different types or ranks, we would just need to find the common type between the 2 and perform the comparison.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

As you said before, the extra bit would not even matter with full precision since it would be trimmed anyway when converting from the common type to the result, and rounding can be either up or down.

Note that this is only true if the unsigned value has a scale that's greater than or equal to the scale of the result type, which is true for same-rank addition but not necessarily with mixed-rank.

For multiplication and division, the result could technically be calculated also without having to cast between both sides, although for using the fixed point mul/div intrinsics, both operands would need to be casted to a common type with the same scale anyway. If a bit does end up getting dropped when casting an operand from unsigned to signed in (1), that dropped bit would not be representable when casting to the result anyway and the standard still allows us to return a rounded result.

In a _Fract multiplication (or a mixed-rank multiplication), the extra bit of precision can affect higher bits in the result. Consider an 8-bit format where you're multiplying unsigned 000.00001 with signed +010.0000: if you do this multiplication in full precision, the result is +000.0001, but if you do the conversion first, the result is 0.

On some level, it just seems really strange to me that the spec calls for doing arbitrary-precision arithmetic and then converting to the result type for every situation except a signed/unsigned mismatch.

[ebevhan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Wait, wait... You might be on to something.

The spec says in 4.1.4:

In order to get useful and attainable results, the usual arithmetic conversions do not apply to the combination of an integer type and a fixed-point type, or the combination of two fixed-point types.
In these cases:

1. the result of the operation is calculated using the values of the two operands, with their full precision;
2. if one operand has signed fixed-point type and the other operand has unsigned fixed-point type, then the unsigned fixed-point operand is converted to its corresponding signed fixed-point type and the resulting type is the type of the converted operand;
3. the result type is the type with the highest rank, [...]

(1) comes before (2); not after. In other words, shouldn't the true result of the operation be computed before the unsigned-signed correction? Point 2 is just worded really oddly, with 'operand is converted' and 'resulting type'.

It says that "the unsigned fixed-point operand is converted to its corresponding signed fixed-point type and the resulting type is the type of the converted operand", but this doesn't mean that the resulting converted operand is actually used, only its type is. This means that for the purpose of result type calculation in (3), the converted operand type from (2) will be used as that operand's type, rather than the unsigned type itself. Or?

The wording here is very hard to interpret.

[rjmccall]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Sure, I think you could make an argument for (2) only being meant to participate in determining the result type. At the very least, that seems more consistent, especially with the behavior for fixed-point + integer operations. Do you have any existing contact with the committee?

[ebevhan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

Do you have any existing contact with the committee?

I don't, no. Do you know who to contact regarding this sort of thing?

For the record, I just tested this with avr-gcc, which supports the E-C fixed-point (without padding on unsigned):

// 0.00390625 is 2*-8.
short _Accum calc = (unsigned short _Accum)0.00390625uk + (signed short _Accum)2.0k;

This produces:

calc:
        .byte   0
        .byte   1

If the unsigned was converted to signed (Q8.7) first, it should have had its bit stripped.

[leonardchan]

clang/lib/Sema/SemaExpr.cpp

1304 ↗

(On Diff #175965)

After thinking about it, it seems more sensible to actually cast the type from unsigned to signed in (2) since that would go against (1). I also looked into contacting the committee but couldn't find any relevant emails.

Updated so that when determining the resulting type, the unsigned fixed point type is converted to that of the signed fixed point operand before comparing ranks. This does not cast the unsigned operand to the signed operand. Now the only casting done is converting to the common type.

[rjmccall]

Okay, thanks, that makes sense to me.

I'll ask around to find a way to contact the C committee.

[leonardchan]

In D53738#1320936, @rjmccall wrote:

Okay, thanks, that makes sense to me.

I'll ask around to find a way to contact the C committee.

@rjmccall Any updates on this? If we aren't able to find a way to contact anyone affiliated with publishing the spec, we could at least double check against avr-gcc for consistency regarding semantics.

[rjmccall]

We sent the question out, but we haven't gotten a response yet. I think going forward under the idea that this just changes the type but does the operation on the original operand types is the right way to go forward in the short term.

[leonardchan]

In D53738#1325998, @rjmccall wrote:

We sent the question out, but we haven't gotten a response yet. I think going forward under the idea that this just changes the type but does the operation on the original operand types is the right way to go forward in the short term.

Ok. Do you think it would be safe to submit this patch currently with the operations handled as you mentioned, or should we wait until we get a response and I can attach future revisions to this patch?

[rjmccall]

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

[leonardchan]

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

[rjmccall]

In D53738#1333276, @leonardchan wrote:

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

I just have a coworker who's part of the committee. I think you might be over-opimistic about how quickly things get answered with the C committee, though. We should not allow our work to be blocked waiting for a response.

[rjmccall]

In D53738#1333372, @rjmccall wrote:

In D53738#1333276, @leonardchan wrote:

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

I just have a coworker who's part of the committee. I think you might be over-opimistic about how quickly things get answered with the C committee, though. We should not allow our work to be blocked waiting for a response.

The committee was less helpful than I hoped, but what I decided to take away from their response was that we should not artificially lose precision here by separating the signedness conversion from the operation.

[leonardchan]

In D53738#1358249, @rjmccall wrote:

In D53738#1333372, @rjmccall wrote:

In D53738#1333276, @leonardchan wrote:

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

I just have a coworker who's part of the committee. I think you might be over-opimistic about how quickly things get answered with the C committee, though. We should not allow our work to be blocked waiting for a response.

The committee was less helpful than I hoped, but what I decided to take away from their response was that we should not artificially lose precision here by separating the signedness conversion from the operation.

Understood. I imagine this is a good way to proceed still. Currently in this patch, the signedness only affects determining the resulting type and does not change the operands during the operation. Is there anything else with this patch that should be addressed, or is it still fine to proceed without committing this patch yet?

[rjmccall]

I think that's the right direction, yeah.

I thought I told you it was fine to commit this patch under that assumption awhile ago. Did I just never click "accept"?

[leonardchan]

In D53738#1359135, @rjmccall wrote:

I think that's the right direction, yeah.

I thought I told you it was fine to commit this patch under that assumption awhile ago. Did I just never click "accept"?

Whoops. I don't think there was an accept, so I kept this patch on hold.

cfe/trunk/include/clang/AST/ASTContext.h

public:
   // accepts fixed point types and returns the corresponding unsigned type for
   // a given fixed point type.
   QualType getCorrespondingUnsignedType(QualType T) const;
 
   // Per ISO N1169, this method accepts fixed point types and returns the
   // corresponding saturated type for a given fixed point type.
   QualType getCorrespondingSaturatedType(QualType Ty) const;
 
+  // This method accepts fixed point types and returns the corresponding signed
+  // type. Unlike getCorrespondingUnsignedType(), this only accepts unsigned
+  // fixed point types because there are unsigned integer types like bool and
+  // char8_t that don't have signed equivalents.
+  QualType getCorrespondingSignedFixedPointType(QualType Ty) const;
+
   //===--------------------------------------------------------------------===//
   //                    Integer Values
   //===--------------------------------------------------------------------===//
 
   /// Make an APSInt of the appropriate width and signedness for the
   /// given \p Value and integer \p Type.
   llvm::APSInt MakeIntValue(uint64_t Value, QualType Type) const {
     // If Type is a signed integer type larger than 64 bits, we need to be sure

cfe/trunk/include/clang/AST/Type.h

#include "clang/Basic/OpenCLExtensionTypes.def"
   /// Determines whether this is an integer type that is unsigned or an
   /// enumeration types whose underlying type is a unsigned integer type.
   bool isUnsignedIntegerOrEnumerationType() const;
 
   /// Return true if this is a fixed point type according to
   /// ISO/IEC JTC1 SC22 WG14 N1169.
   bool isFixedPointType() const;
 
+  /// Return true if this is a fixed point or integer type.
+  bool isFixedPointOrIntegerType() const;
+
   /// Return true if this is a saturated fixed point type according to
   /// ISO/IEC JTC1 SC22 WG14 N1169. This type can be signed or unsigned.
   bool isSaturatedFixedPointType() const;
 
   /// Return true if this is a saturated fixed point type according to
   /// ISO/IEC JTC1 SC22 WG14 N1169. This type can be signed or unsigned.
   bool isUnsaturatedFixedPointType() const;
 

 inline bool Type::isFixedPointType() const {
   if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) {
     return BT->getKind() >= BuiltinType::ShortAccum &&
            BT->getKind() <= BuiltinType::SatULongFract;
   }
   return false;
 }
 
+inline bool Type::isFixedPointOrIntegerType() const {
+  return isFixedPointType() || isIntegerType();
+}
+
 inline bool Type::isSaturatedFixedPointType() const {
   if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) {
     return BT->getKind() >= BuiltinType::SatShortAccum &&
            BT->getKind() <= BuiltinType::SatULongFract;
   }
   return false;
 }
 

cfe/trunk/include/clang/Basic/FixedPoint.h

 /// types described in ISO/IEC JTC1 SC22 WG14 N1169 starting at clause 4.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_BASIC_FIXEDPOINT_H
 #define LLVM_CLANG_BASIC_FIXEDPOINT_H
 
 #include "llvm/ADT/APSInt.h"
+#include "llvm/Support/raw_ostream.h"
 
 namespace clang {
 
 class ASTContext;
 class QualType;
 
 /// The fixed point semantics work similarly to llvm::fltSemantics. The width
 /// specifies the whole bit width of the underlying scaled integer (with padding
 /// if any). The scale represents the number of fractional bits in this type.
 /// When HasUnsignedPadding is true and this type is signed, the first bit
 /// in the value this represents is treaded as padding.
 class FixedPointSemantics {
 public:
   FixedPointSemantics(unsigned Width, unsigned Scale, bool IsSigned,
                       bool IsSaturated, bool HasUnsignedPadding)
       : Width(Width), Scale(Scale), IsSigned(IsSigned),
         IsSaturated(IsSaturated), HasUnsignedPadding(HasUnsignedPadding) {
     assert(Width >= Scale && "Not enough room for the scale");
+    assert(!(IsSigned && HasUnsignedPadding) &&
+           "Cannot have unsigned padding on a signed type.");
   }
 
   unsigned getWidth() const { return Width; }
   unsigned getScale() const { return Scale; }
   bool isSigned() const { return IsSigned; }
   bool isSaturated() const { return IsSaturated; }
   bool hasUnsignedPadding() const { return HasUnsignedPadding; }
 
   void setSaturated(bool Saturated) { IsSaturated = Saturated; }
 
+  /// Return the number of integral bits represented by these semantics. These
+  /// are separate from the fractional bits and do not include the sign or
+  /// padding bit.
   unsigned getIntegralBits() const {
     if (IsSigned || (!IsSigned && HasUnsignedPadding))
       return Width - Scale - 1;
     else
       return Width - Scale;
   }
 
+  /// Return the FixedPointSemantics that allows for calculating the full
+  /// precision semantic that can precisely represent the precision and ranges
+  /// of both input values. This does not compute the resulting semantics for a
+  /// given binary operation.
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics &Other) const;
+
+  /// Return the FixedPointSemantics for an integer type.
+  static FixedPointSemantics GetIntegerSemantics(unsigned Width,
+                                                 bool IsSigned) {
+    return FixedPointSemantics(Width, /*Scale=*/0, IsSigned,
+                               /*IsSaturated=*/false,
+                               /*HasUnsignedPadding=*/false);
+  }
+
 private:
   unsigned Width;
   unsigned Scale;
   bool IsSigned;
   bool IsSaturated;
   bool HasUnsignedPadding;
 };
 

cfe/trunk/lib/AST/ASTContext.cpp

switch (BT->getKind()) {
     case BuiltinType::SatUFract:
     case BuiltinType::ULongFract:
     case BuiltinType::SatULongFract:
       return 0;
   }
 }
 
 FixedPointSemantics ASTContext::getFixedPointSemantics(QualType Ty) const {
-  assert(Ty->isFixedPointType());
+  assert(Ty->isFixedPointType() ||
+         Ty->isIntegerType() && "Can only get the fixed point semantics for a "
+                                "fixed point or integer type.");
+  if (Ty->isIntegerType())
+    return FixedPointSemantics::GetIntegerSemantics(getIntWidth(Ty),
+                                                    Ty->isSignedIntegerType());
+
   bool isSigned = Ty->isSignedFixedPointType();
   return FixedPointSemantics(
       static_cast<unsigned>(getTypeSize(Ty)), getFixedPointScale(Ty), isSigned,
       Ty->isSaturatedFixedPointType(),
       !isSigned && getTargetInfo().doUnsignedFixedPointTypesHavePadding());
 }
 
 APFixedPoint ASTContext::getFixedPointMax(QualType Ty) const {
   assert(Ty->isFixedPointType());
   return APFixedPoint::getMax(getFixedPointSemantics(Ty));
 }
 
 APFixedPoint ASTContext::getFixedPointMin(QualType Ty) const {
   assert(Ty->isFixedPointType());
   return APFixedPoint::getMin(getFixedPointSemantics(Ty));
 }
+
+QualType ASTContext::getCorrespondingSignedFixedPointType(QualType Ty) const {
+  assert(Ty->isUnsignedFixedPointType() &&
+         "Expected unsigned fixed point type");
+  const auto *BTy = Ty->getAs<BuiltinType>();
+
+  switch (BTy->getKind()) {
+  case BuiltinType::UShortAccum:
+    return ShortAccumTy;
+  case BuiltinType::UAccum:
+    return AccumTy;
+  case BuiltinType::ULongAccum:
+    return LongAccumTy;
+  case BuiltinType::SatUShortAccum:
+    return SatShortAccumTy;
+  case BuiltinType::SatUAccum:
+    return SatAccumTy;
+  case BuiltinType::SatULongAccum:
+    return SatLongAccumTy;
+  case BuiltinType::UShortFract:
+    return ShortFractTy;
+  case BuiltinType::UFract:
+    return FractTy;
+  case BuiltinType::ULongFract:
+    return LongFractTy;
+  case BuiltinType::SatUShortFract:
+    return SatShortFractTy;
+  case BuiltinType::SatUFract:
+    return SatFractTy;
+  case BuiltinType::SatULongFract:
+    return SatLongFractTy;
+  default:
+    llvm_unreachable("Unexpected unsigned fixed point type");
+  }
+}

cfe/trunk/lib/Basic/FixedPoint.cpp

APFixedPoint APFixedPoint::getMax(const FixedPointSemantics &Sema) {
   return APFixedPoint(Val, Sema);
 }
 
 APFixedPoint APFixedPoint::getMin(const FixedPointSemantics &Sema) {
   auto Val = llvm::APSInt::getMinValue(Sema.getWidth(), !Sema.isSigned());
   return APFixedPoint(Val, Sema);
 }
 
+FixedPointSemantics FixedPointSemantics::getCommonSemantics(
+    const FixedPointSemantics &Other) const {
+  unsigned CommonScale = std::max(getScale(), Other.getScale());
+  unsigned CommonWidth =
+      std::max(getIntegralBits(), Other.getIntegralBits()) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();
+  bool ResultIsSaturated = isSaturated() || Other.isSaturated();
+  bool ResultHasUnsignedPadding = false;
+  if (!ResultIsSigned) {
+    // Both are unsigned.
+    ResultHasUnsignedPadding = hasUnsignedPadding() &&
+                               Other.hasUnsignedPadding() && !ResultIsSaturated;
+  }
+
+  // If the result is signed, add an extra bit for the sign. Otherwise, if it is
+  // unsigned and has unsigned padding, we only need to add the extra padding
+  // bit back if we are not saturating.
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+    CommonWidth++;
+
+  return FixedPointSemantics(CommonWidth, CommonScale, ResultIsSigned,
+                             ResultIsSaturated, ResultHasUnsignedPadding);
+}
+
 }  // namespace clang

cfe/trunk/lib/CodeGen/CGExprScalar.cpp

struct BinOpInfo {
 
   /// Check if the binop can result in a float division by zero.
   bool mayHaveFloatDivisionByZero() const {
     if (isDivremOp())
       if (auto *CFP = dyn_cast<llvm::ConstantFP>(RHS))
         return CFP->isZero();
     return true;
   }
+
+  /// Check if either operand is a fixed point type, in which case, this
+  /// operation did not follow usual arithmetic conversion and both operands may
+  /// not be the same.
+  bool isFixedPointBinOp() const {
+    return isa<BinaryOperator>(E) && Ty->isFixedPointType();
+  }
 };
 
 static bool MustVisitNullValue(const Expr *E) {
   // If a null pointer expression's type is the C++0x nullptr_t, then
   // it's not necessarily a simple constant and it must be evaluated
   // for its potential side effects.
   return E->getType()->isNullPtrType();
 }
public:
   };
   Value *
   EmitScalarConversion(Value *Src, QualType SrcTy, QualType DstTy,
                        SourceLocation Loc,
                        ScalarConversionOpts Opts = ScalarConversionOpts());
 
   Value *EmitFixedPointConversion(Value *Src, QualType SrcTy, QualType DstTy,
                                   SourceLocation Loc);
+  Value *EmitFixedPointConversion(Value *Src, FixedPointSemantics &SrcFixedSema,
+                                  FixedPointSemantics &DstFixedSema,
+                                  SourceLocation Loc);
 
   /// Emit a conversion from the specified complex type to the specified
   /// destination type, where the destination type is an LLVM scalar type.
   Value *EmitComplexToScalarConversion(CodeGenFunction::ComplexPairTy Src,
                                        QualType SrcTy, QualType DstTy,
                                        SourceLocation Loc);
 
   /// EmitNullValue - Emit a value that corresponds to null for the given type.
public:
   }
   Value *EmitXor(const BinOpInfo &Ops) {
     return Builder.CreateXor(Ops.LHS, Ops.RHS, "xor");
   }
   Value *EmitOr (const BinOpInfo &Ops) {
     return Builder.CreateOr(Ops.LHS, Ops.RHS, "or");
   }
 
+  // Helper functions for fixed point binary operations.
+  Value *EmitFixedPointAdd(const BinOpInfo &Ops);
+
   BinOpInfo EmitBinOps(const BinaryOperator *E);
   LValue EmitCompoundAssignLValue(const CompoundAssignOperator *E,
                             Value *(ScalarExprEmitter::*F)(const BinOpInfo &),
                                   Value *&Result);
 
   Value *EmitCompoundAssign(const CompoundAssignOperator *E,
                             Value *(ScalarExprEmitter::*F)(const BinOpInfo &));
 
EmitIntegerSignChangeCheck(Src, NoncanonicalSrcType, Res,
                                NoncanonicalDstType, Loc);
 
   return Res;
 }
 
 Value *ScalarExprEmitter::EmitFixedPointConversion(Value *Src, QualType SrcTy,
                                                    QualType DstTy,
                                                    SourceLocation Loc) {
-  using llvm::APInt;
-  using llvm::ConstantInt;
-  using llvm::Value;
-
   assert(SrcTy->isFixedPointType());
   assert(DstTy->isFixedPointType());
 
   FixedPointSemantics SrcFPSema =
       CGF.getContext().getFixedPointSemantics(SrcTy);
   FixedPointSemantics DstFPSema =
       CGF.getContext().getFixedPointSemantics(DstTy);
+  return EmitFixedPointConversion(Src, SrcFPSema, DstFPSema, Loc);
+}
+
+Value *ScalarExprEmitter::EmitFixedPointConversion(
+    Value *Src, FixedPointSemantics &SrcFPSema, FixedPointSemantics &DstFPSema,
+    SourceLocation Loc) {
+  using llvm::APInt;
+  using llvm::ConstantInt;
+  using llvm::Value;
+
   unsigned SrcWidth = SrcFPSema.getWidth();
   unsigned DstWidth = DstFPSema.getWidth();
   unsigned SrcScale = SrcFPSema.getScale();
   unsigned DstScale = DstFPSema.getScale();
   bool SrcIsSigned = SrcFPSema.isSigned();
   bool DstIsSigned = DstFPSema.isSigned();
 
   llvm::Type *DstIntTy = Builder.getIntNTy(DstWidth);
if (!DstFPSema.isSaturated()) {
     Result = Builder.CreateIntCast(Result, DstIntTy, SrcIsSigned, "resize");
 
     // Upscale.
     if (DstScale > SrcScale)
       Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
   } else {
     // Adjust the number of fractional bits.
     if (DstScale > SrcScale) {
-      ResultWidth = SrcWidth + DstScale - SrcScale;
+      // Compare to DstWidth to prevent resizing twice.
+      ResultWidth = std::max(SrcWidth + DstScale - SrcScale, DstWidth);
       llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);
       Result = Builder.CreateIntCast(Result, UpscaledTy, SrcIsSigned, "resize");
       Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
     } else if (DstScale < SrcScale) {
       Result = SrcIsSigned ?
           Builder.CreateAShr(Result, SrcScale - DstScale, "downscale") :
           Builder.CreateLShr(Result, SrcScale - DstScale, "downscale");
     }
if (SrcIsSigned && (LessIntBits || !DstIsSigned)) {
       Value *Min = ConstantInt::get(
           CGF.getLLVMContext(),
           APFixedPoint::getMin(DstFPSema).getValue().extOrTrunc(ResultWidth));
       Value *TooLow = Builder.CreateICmpSLT(Result, Min);
       Result = Builder.CreateSelect(TooLow, Min, Result, "satmin");
     }
 
     // Resize the integer part to get the final destination size.
+    if (ResultWidth != DstWidth)
       Result = Builder.CreateIntCast(Result, DstIntTy, SrcIsSigned, "resize");
   }
   return Result;
 }
 
 /// Emit a conversion from the specified complex type to the specified
 /// destination type, where the destination type is an LLVM scalar type.
 Value *ScalarExprEmitter::EmitComplexToScalarConversion(
     CodeGenFunction::ComplexPairTy Src, QualType SrcTy, QualType DstTy,
if (op.LHS->getType()->isFPOrFPVectorTy()) {
     // Try to form an fmuladd.
     if (Value *FMulAdd = tryEmitFMulAdd(op, CGF, Builder))
       return FMulAdd;
 
     Value *V = Builder.CreateFAdd(op.LHS, op.RHS, "add");
     return propagateFMFlags(V, op);
   }
 
+  if (op.isFixedPointBinOp())
+    return EmitFixedPointAdd(op);
+
   return Builder.CreateAdd(op.LHS, op.RHS, "add");
 }
 
+/// The resulting value must be calculated with exact precision, so the operands
+/// may not be the same type.
+Value *ScalarExprEmitter::EmitFixedPointAdd(const BinOpInfo &op) {
+  using llvm::APSInt;
+  using llvm::ConstantInt;
+
+  const auto *BinOp = cast<BinaryOperator>(op.E);
+  assert(BinOp->getOpcode() == BO_Add && "Expected operation to be addition");
+
+  // The result is a fixed point type and at least one of the operands is fixed
+  // point while the other is either fixed point or an int. This resulting type
+  // should be determined by Sema::handleFixedPointConversions().
+  QualType ResultTy = op.Ty;
+  QualType LHSTy = BinOp->getLHS()->getType();
+  QualType RHSTy = BinOp->getRHS()->getType();
+  ASTContext &Ctx = CGF.getContext();
+  Value *LHS = op.LHS;
+  Value *RHS = op.RHS;
+
+  auto LHSFixedSema = Ctx.getFixedPointSemantics(LHSTy);
+  auto RHSFixedSema = Ctx.getFixedPointSemantics(RHSTy);
+  auto ResultFixedSema = Ctx.getFixedPointSemantics(ResultTy);
+  auto CommonFixedSema = LHSFixedSema.getCommonSemantics(RHSFixedSema);
+
+  // Convert the operands to the full precision type.
+  Value *FullLHS = EmitFixedPointConversion(LHS, LHSFixedSema, CommonFixedSema,
+                                            BinOp->getExprLoc());
+  Value *FullRHS = EmitFixedPointConversion(RHS, RHSFixedSema, CommonFixedSema,
+                                            BinOp->getExprLoc());
+
+  // Perform the actual addition.
+  Value *Result;
+  if (ResultFixedSema.isSaturated()) {
+    llvm::Intrinsic::ID IID = ResultFixedSema.isSigned()
+                                  ? llvm::Intrinsic::sadd_sat
+                                  : llvm::Intrinsic::uadd_sat;
+    Result = Builder.CreateBinaryIntrinsic(IID, FullLHS, FullRHS);
+  } else {
+    Result = Builder.CreateAdd(FullLHS, FullRHS);
+  }
+
+  // Convert to the result type.
+  return EmitFixedPointConversion(Result, CommonFixedSema, ResultFixedSema,
+                                  BinOp->getExprLoc());
+}
+
 Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
   // The LHS is always a pointer if either side is.
   if (!op.LHS->getType()->isPointerTy()) {
     if (op.Ty->isSignedIntegerOrEnumerationType()) {
       switch (CGF.getLangOpts().getSignedOverflowBehavior()) {
       case LangOptions::SOB_Defined:
         return Builder.CreateSub(op.LHS, op.RHS, "sub");
       case LangOptions::SOB_Undefined:

cfe/trunk/lib/Sema/SemaExpr.cpp

static QualType handleComplexIntConversion(Sema &S, ExprResult &LHS,
   QualType ComplexType = S.Context.getComplexType(ScalarType);
 
   if (!IsCompAssign)
     LHS = S.ImpCastExprToType(LHS.get(), ComplexType,
                               CK_IntegralRealToComplex);
   return ComplexType;
 }
 
+/// Return the rank of a given fixed point or integer type. The value itself
+/// doesn't matter, but the values must be increasing with proper increasing
+/// rank as described in N1169 4.1.1.
+static unsigned GetFixedPointRank(QualType Ty) {
+  const auto *BTy = Ty->getAs<BuiltinType>();
+  assert(BTy && "Expected a builtin type.");
+
+  switch (BTy->getKind()) {
+  case BuiltinType::ShortFract:
+  case BuiltinType::UShortFract:
+  case BuiltinType::SatShortFract:
+  case BuiltinType::SatUShortFract:
+    return 1;
+  case BuiltinType::Fract:
+  case BuiltinType::UFract:
+  case BuiltinType::SatFract:
+  case BuiltinType::SatUFract:
+    return 2;
+  case BuiltinType::LongFract:
+  case BuiltinType::ULongFract:
+  case BuiltinType::SatLongFract:
+  case BuiltinType::SatULongFract:
+    return 3;
+  case BuiltinType::ShortAccum:
+  case BuiltinType::UShortAccum:
+  case BuiltinType::SatShortAccum:
+  case BuiltinType::SatUShortAccum:
+    return 4;
+  case BuiltinType::Accum:
+  case BuiltinType::UAccum:
+  case BuiltinType::SatAccum:
+  case BuiltinType::SatUAccum:
+    return 5;
+  case BuiltinType::LongAccum:
+  case BuiltinType::ULongAccum:
+  case BuiltinType::SatLongAccum:
+  case BuiltinType::SatULongAccum:
+    return 6;
+  default:
+    if (BTy->isInteger())
+      return 0;
+    llvm_unreachable("Unexpected fixed point or integer type");
+  }
+}
+
+/// handleFixedPointConversion - Fixed point operations between fixed
+/// point types and integers or other fixed point types do not fall under
+/// usual arithmetic conversion since these conversions could result in loss
+/// of precsision (N1169 4.1.4). These operations should be calculated with
+/// the full precision of their result type (N1169 4.1.6.2.1).
+static QualType handleFixedPointConversion(Sema &S, QualType LHSTy,
+                                           QualType RHSTy) {
+  assert((LHSTy->isFixedPointType() || RHSTy->isFixedPointType()) &&
+         "Expected at least one of the operands to be a fixed point type");
+  assert((LHSTy->isFixedPointOrIntegerType() ||
+          RHSTy->isFixedPointOrIntegerType()) &&
+         "Special fixed point arithmetic operation conversions are only "
+         "applied to ints or other fixed point types");
+
+  // If one operand has signed fixed-point type and the other operand has
+  // unsigned fixed-point type, then the unsigned fixed-point operand is
+  // converted to its corresponding signed fixed-point type and the resulting
+  // type is the type of the converted operand.
+  if (RHSTy->isSignedFixedPointType() && LHSTy->isUnsignedFixedPointType())
+    LHSTy = S.Context.getCorrespondingSignedFixedPointType(LHSTy);
+  else if (RHSTy->isUnsignedFixedPointType() && LHSTy->isSignedFixedPointType())
+    RHSTy = S.Context.getCorrespondingSignedFixedPointType(RHSTy);
+
+  // The result type is the type with the highest rank, whereby a fixed-point
+  // conversion rank is always greater than an integer conversion rank; if the
+  // type of either of the operands is a saturating fixedpoint type, the result
+  // type shall be the saturating fixed-point type corresponding to the type
+  // with the highest rank; the resulting value is converted (taking into
+  // account rounding and overflow) to the precision of the resulting type.
+  // Same ranks between signed and unsigned types are resolved earlier, so both
+  // types are either signed or both unsigned at this point.
+  unsigned LHSTyRank = GetFixedPointRank(LHSTy);
+  unsigned RHSTyRank = GetFixedPointRank(RHSTy);
+
+  QualType ResultTy = LHSTyRank > RHSTyRank ? LHSTy : RHSTy;
+
+  if (LHSTy->isSaturatedFixedPointType() || RHSTy->isSaturatedFixedPointType())
+    ResultTy = S.Context.getCorrespondingSaturatedType(ResultTy);
+
+  return ResultTy;
+}
+
 /// UsualArithmeticConversions - Performs various conversions that are common to
 /// binary operators (C99 6.3.1.8). If both operands aren't arithmetic, this
 /// routine returns the first non-arithmetic type found. The client is
 /// responsible for emitting appropriate error diagnostics.
 QualType Sema::UsualArithmeticConversions(ExprResult &LHS, ExprResult &RHS,
                                           bool IsCompAssign) {
   if (!IsCompAssign) {
     LHS = UsualUnaryConversions(LHS.get());
if (LHSType->isRealFloatingType() || RHSType->isRealFloatingType())
     return handleFloatConversion(*this, LHS, RHS, LHSType, RHSType,
                                  IsCompAssign);
 
   // Handle GCC complex int extension.
   if (LHSType->isComplexIntegerType() || RHSType->isComplexIntegerType())
     return handleComplexIntConversion(*this, LHS, RHS, LHSType, RHSType,
                                       IsCompAssign);
 
+  if (LHSType->isFixedPointType() || RHSType->isFixedPointType())
+    return handleFixedPointConversion(*this, LHSType, RHSType);
+
   // Finally, we have two differing integer types.
   return handleIntegerConversion<doIntegralCast, doIntegralCast>
            (*this, LHS, RHS, LHSType, RHSType, IsCompAssign);
 }
 
-
 //===----------------------------------------------------------------------===//
 //  Semantic Analysis for various Expression Types
 //===----------------------------------------------------------------------===//
 
 
 ExprResult
 Sema::ActOnGenericSelectionExpr(SourceLocation KeyLoc,
                                 SourceLocation DefaultLoc,

cfe/trunk/test/Frontend/fixed_point_add.c

+// RUN: %clang_cc1 -ffixed-point -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,SIGNED
+// RUN: %clang_cc1 -ffixed-point -fpadding-on-unsigned-fixed-point -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,UNSIGNED
+
+void SignedAddition() {
+  // CHECK-LABEL: SignedAddition
+  short _Accum sa;
+  _Accum a, b, c, d;
+  long _Accum la;
+  unsigned short _Accum usa;
+  unsigned _Accum ua;
+  unsigned long _Accum ula;
+
+  short _Fract sf;
+  _Fract f;
+  long _Fract lf;
+  unsigned short _Fract usf;
+  unsigned _Fract uf;
+  unsigned long _Fract ulf;
+
+  // Same type
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[SA2:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i16 [[SA]], [[SA2]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %sa, align 2
+  sa = sa + sa;
+
+  // To larger scale and larger width
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[A:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[EXT_SA:%[a-z0-9]+]] = sext i16 [[SA]] to i32
+  // CHECK-NEXT: [[SA:%[a-z0-9]+]] = shl i32 [[EXT_SA]], 8
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i32 [[SA]], [[A]]
+  // CHECK-NEXT: store i32 [[SUM]], i32* %a, align 4
+  a = sa + a;
+
+  // To same scale and smaller width
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[SF:%[0-9]+]] = load i8, i8* %sf, align 1
+  // CHECK-NEXT: [[EXT_SF:%[a-z0-9]+]] = sext i8 [[SF]] to i16
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i16 [[SA]], [[EXT_SF]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %sa, align 2
+  sa = sa + sf;
+
+  // To smaller scale and same width.
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[F:%[0-9]+]] = load i16, i16* %f, align 2
+  // CHECK-NEXT: [[EXT_SA:%[a-z0-9]+]] = sext i16 [[SA]] to i24
+  // CHECK-NEXT: [[SA:%[a-z0-9]+]] = shl i24 [[EXT_SA]], 8
+  // CHECK-NEXT: [[EXT_F:%[a-z0-9]+]] = sext i16 [[F]] to i24
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i24 [[SA]], [[EXT_F]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = ashr i24 [[SUM]], 8
+  // CHECK-NEXT: [[TRUNC_RES:%[a-z0-9]+]] = trunc i24 [[RES]] to i16
+  // CHECK-NEXT: store i16 [[TRUNC_RES]], i16* %sa, align 2
+  sa = sa + f;
+
+  // To smaller scale and smaller width
+  // CHECK:      [[A:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[SF:%[0-9]+]] = load i8, i8* %sf, align 1
+  // CHECK-NEXT: [[EXT_SF:%[a-z0-9]+]] = sext i8 [[SF]] to i32
+  // CHECK-NEXT: [[SF:%[a-z0-9]+]] = shl i32 [[EXT_SF]], 8
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i32 [[A]], [[SF]]
+  // CHECK-NEXT: store i32 [[SUM]], i32* %a, align 4
+  a = a + sf;
+
+  // To larger scale and same width
+  // CHECK:      [[A:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[LF:%[0-9]+]] = load i32, i32* %lf, align 4
+  // CHECK-NEXT: [[EXT_A:%[a-z0-9]+]] = sext i32 [[A]] to i48
+  // CHECK-NEXT: [[A:%[a-z0-9]+]] = shl i48 [[EXT_A]], 16
+  // CHECK-NEXT: [[EXT_LF:%[a-z0-9]+]] = sext i32 [[LF]] to i48
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i48 [[A]], [[EXT_LF]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = ashr i48 [[SUM]], 16
+  // CHECK-NEXT: [[TRUNC_RES:%[a-z0-9]+]] = trunc i48 [[RES]] to i32
+  // CHECK-NEXT: store i32 [[TRUNC_RES]], i32* %a, align 4
+  a = a + lf;
+
+  // With corresponding unsigned type
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // SIGNED-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i17
+  // SIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i17 [[SA_EXT]], 1
+  // SIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i17
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i17 [[SA]], [[USA_EXT]]
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = ashr i17 [[SUM]], 1
+  // SIGNED-NEXT: [[SUM:%[a-z0-9]+]] = trunc i17 [[RESULT]] to i16
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i16 [[SA]], [[USA]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %sa, align 2
+  sa = sa + usa;
+
+  // With unsigned of larger scale
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[USA:%[0-9]+]] = load i32, i32* %ua, align 4
+  // SIGNED-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i33
+  // SIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i33 [[SA_EXT]], 9
+  // SIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i32 [[USA]] to i33
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i33 [[SA]], [[USA_EXT]]
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = ashr i33 [[SUM]], 1
+  // SIGNED-NEXT: [[SUM:%[a-z0-9]+]] = trunc i33 [[RESULT]] to i32
+  // UNSIGNED-NEXT: [[EXT_SA:%[a-z0-9]+]] = sext i16 [[SA]] to i32
+  // UNSIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i32 [[EXT_SA]], 8
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i32 [[SA]], [[USA]]
+  // CHECK-NEXT: store i32 [[SUM]], i32* %a, align 4
+  a = sa + ua;
+
+  // With unsigned of smaller width
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[USF:%[0-9]+]] = load i8, i8* %usf, align 1
+  // SIGNED-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i17
+  // SIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i17 [[SA_EXT]], 1
+  // SIGNED-NEXT: [[USF_EXT:%[a-z0-9]+]] = zext i8 [[USF]] to i17
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i17 [[SA]], [[USF_EXT]]
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = ashr i17 [[SUM]], 1
+  // SIGNED-NEXT: [[SUM:%[a-z0-9]+]] = trunc i17 [[RESULT]] to i16
+  // UNSIGNED-NEXT: [[EXT_USF:%[a-z0-9]+]] = zext i8 [[USF]] to i16
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i16 [[SA]], [[EXT_USF]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %sa, align 2
+  sa = sa + usf;
+
+  // With unsigned of larger width and smaller scale
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[ULF:%[0-9]+]] = load i32, i32* %ulf, align 4
+  // SIGNED-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i41
+  // SIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i41 [[SA_EXT]], 25
+  // SIGNED-NEXT: [[ULF_EXT:%[a-z0-9]+]] = zext i32 [[ULF]] to i41
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i41 [[SA]], [[ULF_EXT]]
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = ashr i41 [[SUM]], 25
+  // SIGNED-NEXT: [[RES_TRUNC:%[a-z0-9]+]] = trunc i41 [[RESULT]] to i16
+  // UNSIGNED-NEXT: [[EXT_SA:%[a-z0-9]+]] = sext i16 [[SA]] to i40
+  // UNSIGNED-NEXT: [[SA:%[a-z0-9]+]] = shl i40 [[EXT_SA]], 24
+  // UNSIGNED-NEXT: [[EXT_ULF:%[a-z0-9]+]] = zext i32 [[ULF]] to i40
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[SA]], [[EXT_ULF]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = ashr i40 [[SUM]], 24
+  // UNSIGNED-NEXT: [[RES_TRUNC:%[a-z0-9]+]] = trunc i40 [[RES]] to i16
+  // CHECK-NEXT: store i16 [[RES_TRUNC]], i16* %sa, align 2
+  sa = sa + ulf;
+
+  // Chained additions of the same signed type should result in the same
+  // semantics width.
+  // CHECK:      [[A:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[B:%[0-9]+]] = load i32, i32* %b, align 4
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i32 [[A]], [[B]]
+  // CHECK-NEXT: [[C:%[0-9]+]] = load i32, i32* %c, align 4
+  // CHECK-NEXT: [[SUM2:%[0-9]+]] = add i32 [[SUM]], [[C]]
+  // CHECK-NEXT: [[D:%[0-9]+]] = load i32, i32* %d, align 4
+  // CHECK-NEXT: [[SUM3:%[0-9]+]] = add i32 [[SUM2]], [[D]]
+  // CHECK-NEXT: store i32 [[SUM3]], i32* %a, align 4
+  a = a + b + c + d;
+}
+
+void UnsignedAddition() {
+  // CHECK-LABEL: UnsignedAddition
+  unsigned short _Accum usa;
+  unsigned _Accum ua;
+  unsigned long _Accum ula;
+
+  unsigned short _Fract usf;
+  unsigned _Fract uf;
+  unsigned long _Fract ulf;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[USA2:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i16 [[USA]], [[USA2]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %usa, align 2
+  usa = usa + usa;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[UA:%[0-9]+]] = load i32, i32* %ua, align 4
+  // CHECK-NEXT: [[EXT_USA:%[a-z0-9]+]] = zext i16 [[USA]] to i32
+  // CHECK-NEXT: [[USA:%[a-z0-9]+]] = shl i32 [[EXT_USA]], 8
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i32 [[USA]], [[UA]]
+  // CHECK-NEXT: store i32 [[SUM]], i32* %ua, align 4
+  ua = usa + ua;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[USF:%[0-9]+]] = load i8, i8* %usf, align 1
+  // CHECK-NEXT: [[EXT_USF:%[a-z0-9]+]] = zext i8 [[USF]] to i16
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i16 [[USA]], [[EXT_USF]]
+  // CHECK-NEXT: store i16 [[SUM]], i16* %usa, align 2
+  usa = usa + usf;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[UF:%[0-9]+]] = load i16, i16* %uf, align 2
+  // CHECK-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i24
+  // CHECK-NEXT: [[USA:%[a-z0-9]+]] = shl i24 [[USA_EXT]], 8
+  // CHECK-NEXT: [[UF_EXT:%[a-z0-9]+]] = zext i16 [[UF]] to i24
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i24 [[USA]], [[UF_EXT]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = lshr i24 [[SUM]], 8
+  // CHECK-NEXT: [[RES_TRUNC:%[a-z0-9]+]] = trunc i24 [[RES]] to i16
+  // CHECK-NEXT: store i16 [[RES_TRUNC]], i16* %usa, align 2
+  usa = usa + uf;
+}
+
+void IntAddition() {
+  // CHECK-LABEL: IntAddition
+  short _Accum sa;
+  _Accum a;
+  unsigned short _Accum usa;
+  _Sat short _Accum sa_sat;
+  int i;
+  unsigned int ui;
+  long _Fract lf;
+  _Bool b;
+
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %i, align 4
+  // CHECK-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i39
+  // CHECK-NEXT: [[I_EXT:%[a-z0-9]+]] = sext i32 [[I]] to i39
+  // CHECK-NEXT: [[I:%[a-z0-9]+]] = shl i39 [[I_EXT]], 7
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i39 [[SA_EXT]], [[I]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SUM]] to i16
+  // CHECK-NEXT: store i16 [[RES]], i16* %sa, align 2
+  sa = sa + i;
+
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[UI:%[0-9]+]] = load i32, i32* %ui, align 4
+  // CHECK-NEXT: [[SA_EXT:%[a-z0-9]+]] = sext i16 [[SA]] to i40
+  // CHECK-NEXT: [[UI_EXT:%[a-z0-9]+]] = zext i32 [[UI]] to i40
+  // CHECK-NEXT: [[UI:%[a-z0-9]+]] = shl i40 [[UI_EXT]], 7
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i40 [[SA_EXT]], [[UI]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16
+  // CHECK-NEXT: store i16 [[RES]], i16* %sa, align 2
+  sa = sa + ui;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %i, align 4
+  // SIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i40
+  // SIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = sext i32 [[I]] to i40
+  // SIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 8
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[USA_EXT]], [[I]]
+  // SIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i39
+  // UNSIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = sext i32 [[I]] to i39
+  // UNSIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i39 [[I_EXT]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i39 [[USA_EXT]], [[I]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SUM]] to i16
+  // CHECK-NEXT: store i16 [[RES]], i16* %usa, align 2
+  usa = usa + i;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %ui, align 4
+  // SIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i40
+  // SIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i40
+  // SIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 8
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[USA_EXT]], [[I]]
+  // SIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i39
+  // UNSIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i39
+  // UNSIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i39 [[I_EXT]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i39 [[USA_EXT]], [[I]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SUM]] to i16
+  // CHECK-NEXT: store i16 [[RES]], i16* %usa, align 2
+  usa = usa + ui;
+
+  // CHECK:      [[LF:%[0-9]+]] = load i32, i32* %lf, align 4
+  // CHECK-NEXT: [[UI:%[0-9]+]] = load i32, i32* %ui, align 4
+  // CHECK-NEXT: [[LF_EXT:%[a-z0-9]+]] = sext i32 [[LF]] to i64
+  // CHECK-NEXT: [[UI_EXT:%[a-z0-9]+]] = zext i32 [[UI]] to i64
+  // CHECK-NEXT: [[UI:%[a-z0-9]+]] = shl i64 [[UI_EXT]], 31
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i64 [[LF_EXT]], [[UI]]
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = trunc i64 [[SUM]] to i32
+  // CHECK-NEXT: store i32 [[RES]], i32* %lf, align 4
+  lf = lf + ui;
+
+  // CHECK:      [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[BOOL:%[0-9]+]] = load i8, i8* %b, align 1
+  // CHECK-NEXT: [[AS_BOOL:%[a-z0-9]+]] = trunc i8 [[BOOL]] to i1
+  // CHECK-NEXT: [[BOOL_EXT:%[a-z0-9]+]] = zext i1 [[AS_BOOL]] to i32
+  // CHECK-NEXT: [[ACCUM_EXT:%[a-z0-9]+]] = sext i32 [[ACCUM]] to i47
+  // CHECK-NEXT: [[BOOL:%[a-z0-9]+]] = sext i32 [[BOOL_EXT]] to i47
+  // CHECK-NEXT: [[BOOL_EXT:%[a-z0-9]+]] = shl i47 [[BOOL]], 15
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = add i47 [[ACCUM_EXT]], [[BOOL_EXT]]
+  // CHECK-NEXT: [[RESULT:%[a-z0-9]+]] = trunc i47 [[SUM]] to i32
+  // CHECK-NEXT: store i32 [[RESULT]], i32* %a, align 4
+  a = a + b;
+}
+
+void SaturatedAddition() {
+  // CHECK-LABEL: SaturatedAddition
+  short _Accum sa;
+  _Accum a;
+  long _Accum la;
+  unsigned short _Accum usa;
+  unsigned _Accum ua;
+  unsigned long _Accum ula;
+
+  _Sat short _Accum sa_sat;
+  _Sat _Accum a_sat;
+  _Sat long _Accum la_sat;
+  _Sat unsigned short _Accum usa_sat;
+  _Sat unsigned _Accum ua_sat;
+  _Sat unsigned long _Accum ula_sat;
+  _Sat unsigned _Fract uf_sat;
+
+  int i;
+  unsigned int ui;
+
+  // CHECK:      [[SA:%[0-9]+]] = load i16, i16* %sa, align 2
+  // CHECK-NEXT: [[SA_SAT:%[0-9]+]] = load i16, i16* %sa_sat, align 2
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = call i16 @llvm.sadd.sat.i16(i16 [[SA]], i16
+  // [[SA_SAT]])
+  // CHECK-NEXT: store i16 [[SUM]], i16* %sa_sat, align 2
+  sa_sat = sa + sa_sat;
+
+  // CHECK:      [[USA:%[0-9]+]] = load i16, i16* %usa, align 2
+  // CHECK-NEXT: [[USA_SAT:%[0-9]+]] = load i16, i16* %usa_sat, align 2
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = call i16 @llvm.uadd.sat.i16(i16 [[USA]], i16 [[USA_SAT]])
+  // SIGNED-NEXT: store i16 [[SUM]], i16* %usa_sat, align 2
+  // UNSIGNED-NEXT: [[USA_TRUNC:%[a-z0-9]+]] = trunc i16 [[USA]] to i15
+  // UNSIGNED-NEXT: [[USA_SAT_TRUNC:%[a-z0-9]+]] = trunc i16 [[USA_SAT]] to i15
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 [[USA_TRUNC]], i15 [[USA_SAT_TRUNC]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2
+  usa_sat = usa + usa_sat;
+
+  // CHECK:      [[UA:%[0-9]+]] = load i32, i32* %ua, align 4
+  // CHECK-NEXT: [[USA:%[0-9]+]] = load i16, i16* %usa_sat, align 2
+  // SIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i32
+  // SIGNED-NEXT: [[USA:%[a-z0-9]+]] = shl i32 [[USA_EXT]], 8
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = call i32 @llvm.uadd.sat.i32(i32 [[UA]], i32 [[USA]])
+  // SIGNED-NEXT: store i32 [[SUM]], i32* %ua_sat, align 4
+  // UNSIGNED-NEXT: [[UA_TRUNC:%[a-z0-9]+]] = trunc i32 [[UA]] to i31
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i31
+  // UNSIGNED-NEXT: [[USA:%[a-z0-9]+]] = shl i31 [[USA_EXT]], 8
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i31 @llvm.uadd.sat.i31(i31 [[UA_TRUNC]], i31 [[USA]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i31 [[SUM]] to i32
+  // UNSIGNED-NEXT: store i32 [[SUM_EXT]], i32* %ua_sat, align 4
+  ua_sat = ua + usa_sat;
+
+  // CHECK:      [[SA_SAT:%[0-9]+]] = load i16, i16* %sa_sat, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %i, align 4
+  // CHECK-NEXT: [[SA_SAT_EXT:%[a-z0-9]+]] = sext i16 [[SA_SAT]] to i39
+  // CHECK-NEXT: [[I_EXT:%[a-z0-9]+]] = sext i32 [[I]] to i39
+  // CHECK-NEXT: [[I:%[a-z0-9]+]] = shl i39 [[I_EXT]], 7
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = call i39 @llvm.sadd.sat.i39(i39 [[SA_SAT_EXT]], i39 [[I]])
+  // CHECK-NEXT: [[USE_MAX:%[0-9]+]] = icmp sgt i39 [[SUM]], 32767
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = select i1 [[USE_MAX]], i39 32767, i39 [[SUM]]
+  // CHECK-NEXT: [[USE_MIN:%[0-9]+]] = icmp slt i39 [[RES]], -32768
+  // CHECK-NEXT: [[RES2:%[a-z0-9]+]] = select i1 [[USE_MIN]], i39 -32768, i39 [[RES]]
+  // CHECK-NEXT: [[RES3:%[a-z0-9]+]] = trunc i39 [[RES2]] to i16
+  // CHECK-NEXT: store i16 [[RES3]], i16* %sa_sat, align 2
+  sa_sat = sa_sat + i;
+
+  // CHECK:      [[SA_SAT:%[0-9]+]] = load i16, i16* %sa_sat, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %ui, align 4
+  // CHECK-NEXT: [[SA_SAT_EXT:%[a-z0-9]+]] = sext i16 [[SA_SAT]] to i40
+  // CHECK-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i40
+  // CHECK-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 7
+  // CHECK-NEXT: [[SUM:%[0-9]+]] = call i40 @llvm.sadd.sat.i40(i40 [[SA_SAT_EXT]], i40 [[I]])
+  // CHECK-NEXT: [[USE_MAX:%[0-9]+]] = icmp sgt i40 [[SUM]], 32767
+  // CHECK-NEXT: [[RES:%[a-z0-9]+]] = select i1 [[USE_MAX]], i40 32767, i40 [[SUM]]
+  // CHECK-NEXT: [[USE_MIN:%[0-9]+]] = icmp slt i40 [[RES]], -32768
+  // CHECK-NEXT: [[RES2:%[a-z0-9]+]] = select i1 [[USE_MIN]], i40 -32768, i40 [[RES]]
+  // CHECK-NEXT: [[RES3:%[a-z0-9]+]] = trunc i40 [[RES2]] to i16
+  // CHECK-NEXT: store i16 [[RES3]], i16* %sa_sat, align 2
+  sa_sat = sa_sat + ui;
+
+  // CHECK:      [[UF_SAT:%[0-9]+]] = load i16, i16* %uf_sat, align 2
+  // CHECK-NEXT: [[UF_SAT2:%[0-9]+]] = load i16, i16* %uf_sat, align 2
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = call i16 @llvm.uadd.sat.i16(i16 [[UF_SAT]], i16 [[UF_SAT2]])
+  // SIGNED-NEXT: store i16 [[SUM]], i16* %uf_sat, align 2
+  // UNSIGNED-NEXT: [[UF_SAT_TRUNC:%[a-z0-9]+]] = trunc i16 [[UF_SAT]] to i15
+  // UNSIGNED-NEXT: [[UF_SAT_TRUNC2:%[a-z0-9]+]] = trunc i16 [[UF_SAT2]] to i15
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 [[UF_SAT_TRUNC]], i15 [[UF_SAT_TRUNC2]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %uf_sat, align 2
+  uf_sat = uf_sat + uf_sat;
+
+  // CHECK:      [[USA_SAT:%[0-9]+]] = load i16, i16* %usa_sat, align 2
+  // CHECK-NEXT: [[I:%[0-9]+]] = load i32, i32* %i, align 4
+  // SIGNED-NEXT: [[USA_SAT_RESIZE:%[a-z0-9]+]] = zext i16 [[USA_SAT]] to i40
+  // SIGNED-NEXT: [[I_RESIZE:%[a-z0-9]+]] = sext i32 [[I]] to i40
+  // SIGNED-NEXT: [[I_UPSCALE:%[a-z0-9]+]] = shl i40 [[I_RESIZE]], 8
+  // SIGNED-NEXT: [[SUM:%[0-9]+]] = call i40 @llvm.uadd.sat.i40(i40 [[USA_SAT_RESIZE]], i40 [[I_UPSCALE]])
+  // SIGNED-NEXT: [[USE_MAX:%[0-9]+]] = icmp sgt i40 [[SUM]], 65535
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = select i1 [[USE_MAX]], i40 65535, i40 [[SUM]]
+  // SIGNED-NEXT: [[USE_MIN:%[0-9]+]] = icmp slt i40 [[RESULT]], 0
+  // SIGNED-NEXT: [[RESULT2:%[a-z0-9]+]] = select i1 [[USE_MIN]], i40 0, i40 [[RESULT]]
+  // SIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = trunc i40 [[RESULT2]] to i16
+  // UNSIGNED-NEXT: [[USA_SAT_RESIZE:%[a-z0-9]+]] = zext i16 [[USA_SAT]] to i39
+  // UNSIGNED-NEXT: [[I_RESIZE:%[a-z0-9]+]] = sext i32 [[I]] to i39
+  // UNSIGNED-NEXT: [[I_UPSCALE:%[a-z0-9]+]] = shl i39 [[I_RESIZE]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i39 @llvm.uadd.sat.i39(i39 [[USA_SAT_RESIZE]], i39 [[I_UPSCALE]])
+  // UNSIGNED-NEXT: [[USE_MAX:%[0-9]+]] = icmp sgt i39 [[SUM]], 32767
+  // UNSIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = select i1 [[USE_MAX]], i39 32767, i39 [[SUM]]
+  // UNSIGNED-NEXT: [[USE_MIN:%[0-9]+]] = icmp slt i39 [[RESULT]], 0
+  // UNSIGNED-NEXT: [[RESULT2:%[a-z0-9]+]] = select i1 [[USE_MIN]], i39 0, i39 [[RESULT]]
+  // UNSIGNED-NEXT: [[RESULT:%[a-z0-9]+]] = trunc i39 [[RESULT2]] to i16
+  // CHECK-NEXT: store i16 [[RESULT]], i16* %usa_sat, align 2
+  usa_sat = usa_sat + i;
+}

cfe/trunk/test/Frontend/fixed_point_conversions.c

void TestFixedPointCastSaturation() {
   sat_a = sat_f;
   // DEFAULT:      [[FRACT:%[0-9a-z]+]] = load i16, i16* %sat_f, align 2
   // DEFAULT-NEXT: [[FRACT_EXT:%[0-9a-z]+]] = sext i16 [[FRACT]] to i32
   // DEFAULT-NEXT: store i32 [[FRACT_EXT]], i32* %sat_a, align 4
 
   // Only get overflow checking if signed fract to unsigned accum
   sat_ua = sat_sf;
   // DEFAULT:      [[FRACT:%[0-9a-z]+]] = load i8, i8* %sat_sf, align 1
-  // DEFAULT-NEXT: [[FRACT_EXT:%[0-9a-z]+]] = sext i8 [[FRACT]] to i17
-  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i17 [[FRACT_EXT]], 9
-  // DEFAULT-NEXT: [[IS_NEG:%[0-9a-z]+]] = icmp slt i17 [[ACCUM]], 0
-  // DEFAULT-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[IS_NEG]], i17 0, i17 [[ACCUM]]
-  // DEFAULT-NEXT: [[RESULT_EXT:%[0-9a-z]+]] = sext i17 [[RESULT]] to i32
-  // DEFAULT-NEXT: store i32 [[RESULT_EXT]], i32* %sat_ua, align 4
+  // DEFAULT-NEXT: [[FRACT_EXT:%[0-9a-z]+]] = sext i8 [[FRACT]] to i32
+  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i32 [[FRACT_EXT]], 9
+  // DEFAULT-NEXT: [[IS_NEG:%[0-9a-z]+]] = icmp slt i32 [[ACCUM]], 0
+  // DEFAULT-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[IS_NEG]], i32 0, i32 [[ACCUM]]
+  // DEFAULT-NEXT: store i32 [[RESULT]], i32* %sat_ua, align 4
   // SAME:      [[FRACT:%[0-9a-z]+]] = load i8, i8* %sat_sf, align 1
-  // SAME-NEXT: [[FRACT_EXT:%[0-9a-z]+]] = sext i8 [[FRACT]] to i16
-  // SAME-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i16 [[FRACT_EXT]], 8
-  // SAME-NEXT: [[IS_NEG:%[0-9a-z]+]] = icmp slt i16 [[ACCUM]], 0
-  // SAME-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[IS_NEG]], i16 0, i16 [[ACCUM]]
-  // SAME-NEXT: [[RESULT_EXT:%[0-9a-z]+]] = sext i16 [[RESULT]] to i32
-  // SAME-NEXT: store i32 [[RESULT_EXT]], i32* %sat_ua, align 4
+  // SAME-NEXT: [[FRACT_EXT:%[0-9a-z]+]] = sext i8 [[FRACT]] to i32
+  // SAME-NEXT: [[ACCUM:%[0-9a-z]+]] = shl i32 [[FRACT_EXT]], 8
+  // SAME-NEXT: [[IS_NEG:%[0-9a-z]+]] = icmp slt i32 [[ACCUM]], 0
+  // SAME-NEXT: [[RESULT:%[0-9a-z]+]] = select i1 [[IS_NEG]], i32 0, i32 [[ACCUM]]
+  // SAME-NEXT: store i32 [[RESULT]], i32* %sat_ua, align 4
 }
 
 void TestFixedPointCastBetFractAccum() {
   short _Accum sa;
   _Accum a;
   long _Accum la;
   short _Fract sf;
   _Fract f;