[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL351364: [Fixed Point Arithmetic] Fixed Point Addition 
(authored by leonardchan, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53738?vs=176862=182094#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D53738

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/FixedPoint.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/Basic/FixedPoint.cpp
  cfe/trunk/lib/CodeGen/CGExprScalar.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Frontend/fixed_point_add.c
  cfe/trunk/test/Frontend/fixed_point_conversions.c

Index: cfe/trunk/test/Frontend/fixed_point_conversions.c
===
--- cfe/trunk/test/Frontend/fixed_point_conversions.c
+++ cfe/trunk/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: cfe/trunk/test/Frontend/fixed_point_add.c
===
--- cfe/trunk/test/Frontend/fixed_point_add.c
+++ cfe/trunk/test/Frontend/fixed_point_add.c
@@ -0,0 +1,390 @@
+// 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]] 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D53738#1358249 , @rjmccall wrote:

> In D53738#172 , @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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D53738#172 , @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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Okay, thanks, that makes sense to me.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

ebevhan wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > rjmccall wrote:
> > > > leonardchan wrote:
> > > > > rjmccall wrote:
> > > > > > leonardchan wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > leonardchan wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > > > `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.
> > > > > > > > > > > > > > > `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.
> > > > > > > > > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 176862.
leonardchan marked an inline comment as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add.c
@@ -0,0 +1,390 @@
+// 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > rjmccall wrote:
> > > > > leonardchan wrote:
> > > > > > rjmccall wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > ebevhan wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > > `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.
> > > > > > > > > > > > > > `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.
> > > > > > > > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

ebevhan wrote:
> rjmccall wrote:
> > leonardchan wrote:
> > > rjmccall wrote:
> > > > leonardchan wrote:
> > > > > rjmccall wrote:
> > > > > > leonardchan wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > ebevhan wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > > 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.
> > > > > > > > > > > > > `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.
> > > > > > > > > > > > > `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.
> > > > > > > > > > > > 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 
> > > > > > > > > > > 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > rjmccall wrote:
> > > > > leonardchan wrote:
> > > > > > rjmccall wrote:
> > > > > > > ebevhan wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > leonardchan wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > > 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.
> > > > > > > > > > > > `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.
> > > > > > > > > > > > `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.
> > > > > > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

leonardchan wrote:
> rjmccall wrote:
> > leonardchan wrote:
> > > rjmccall wrote:
> > > > leonardchan wrote:
> > > > > rjmccall wrote:
> > > > > > ebevhan wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > leonardchan wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > > `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.
> > > > > > > > > > > `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.
> > > > > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > rjmccall wrote:
> > > > > ebevhan wrote:
> > > > > > rjmccall wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > ebevhan wrote:
> > > > > > > > > > rjmccall wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > `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.
> > > > > > > > > > `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.
> > > > > > > > > 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 
> > > > > > > > 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

leonardchan wrote:
> rjmccall wrote:
> > leonardchan wrote:
> > > rjmccall wrote:
> > > > ebevhan wrote:
> > > > > rjmccall wrote:
> > > > > > leonardchan wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > ebevhan wrote:
> > > > > > > > > rjmccall wrote:
> > > > > > > > > > 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.
> > > > > > > > > `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.
> > > > > > > > > `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.
> > > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > rjmccall wrote:
> > > > > leonardchan wrote:
> > > > > > rjmccall wrote:
> > > > > > > ebevhan wrote:
> > > > > > > > rjmccall wrote:
> > > > > > > > > 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.
> > > > > > > > `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.
> > > > > > > > `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.
> > > > > > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

leonardchan wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > rjmccall wrote:
> > > > leonardchan wrote:
> > > > > rjmccall wrote:
> > > > > > ebevhan wrote:
> > > > > > > rjmccall wrote:
> > > > > > > > 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.
> > > > > > > `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.
> > > > > > > `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.
> > > > > > 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.
> > > > 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 
> 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked an inline comment as done.
leonardchan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > leonardchan wrote:
> > > > rjmccall wrote:
> > > > > ebevhan wrote:
> > > > > > rjmccall wrote:
> > > > > > > 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.
> > > > > > `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.
> > > > > > `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.
> > > > > 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.
> > > 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

ebevhan wrote:
> rjmccall wrote:
> > leonardchan wrote:
> > > rjmccall wrote:
> > > > ebevhan wrote:
> > > > > rjmccall wrote:
> > > > > > 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.
> > > > > `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.
> > > > > `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.
> > > > 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.
> > 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.
> The coercion of unsigned to signed is likely done for pragmatic reasons; if 
> you have a 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > rjmccall wrote:
> > > > > 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.
> > > > `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.
> > > > `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.
> > > 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.
> 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.
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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

leonardchan wrote:
> rjmccall wrote:
> > ebevhan wrote:
> > > rjmccall wrote:
> > > > 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.
> > > `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.
> > > `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.
> > 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.
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics ) const;
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > 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?
> > 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.
> 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.
Yup. Updated the documentation.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > 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.
> > `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.
> > `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.
> 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 176204.
leonardchan marked 6 inline comments as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

ebevhan wrote:
> rjmccall wrote:
> > 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.
> `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.
> `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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> 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.
`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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics ) const;
+

leonardchan wrote:
> rjmccall wrote:
> > 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?
> 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.
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.



Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+CommonWidth++;
+

leonardchan wrote:
> rjmccall wrote:
> > 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?
> 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.
I see.  If that's done consistently then it's fine that it technically slightly 
misrepresents the range of negative values.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3370
+
+  const auto *BinOp = dyn_cast(op.E);
+  assert(BinOp && "Expected the operator to be a binary operator");

Please use `cast`.



Comment at: clang/lib/Sema/SemaExpr.cpp:1237
+  if (Ty->isIntegerType())
+return 0;
+

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.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 175965.
leonardchan marked 14 inline comments as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics ) const;
+

rjmccall wrote:
> 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?
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.



Comment at: clang/lib/AST/ASTContext.cpp:10489
+return FixedPointSemantics::GetIntegerSemantics(getIntWidth(Ty),
+Ty->isSignedIntegerType());
+

rjmccall wrote:
> Does `getIntWidth` do the thing you want for `bool`?
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.



Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+CommonWidth++;
+

rjmccall wrote:
> 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?
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/AST/ASTContext.h:2638
+  // type.
+  QualType getCorrespondingSignedFixedPointType(QualType Ty) const;
+

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.



Comment at: clang/include/clang/Basic/FixedPoint.h:41
+  assert(!HasUnsignedPadding &&
+ "Cannot have unsigned padding on a signed type.");
   }

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



Comment at: clang/include/clang/Basic/FixedPoint.h:67
+  FixedPointSemantics
+  getCommonSemantics(const FixedPointSemantics ) const;
+

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?



Comment at: clang/include/clang/Basic/FixedPoint.h:70
+  /// Return the FixedPointSemantics for an integer type.
+  static FixedPointSemantics GetIntegerSemantics(unsigned Width, bool 
IsSigned);
+

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



Comment at: clang/lib/AST/ASTContext.cpp:10489
+return FixedPointSemantics::GetIntegerSemantics(getIntWidth(Ty),
+Ty->isSignedIntegerType());
+

Does `getIntWidth` do the thing you want for `bool`?



Comment at: clang/lib/Basic/FixedPoint.cpp:141
+  if (ResultIsSigned || ResultHasUnsignedPadding)
+CommonWidth++;
+

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?


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked an inline comment as done.
leonardchan added a comment.

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?




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > 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.
> > > > 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`?
> > > 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).
> > 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.
> 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`.
Got it. Thanks!


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

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




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > 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.
> > > 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`?
> > 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).
> 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.
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`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked an inline comment as done.
leonardchan added a comment.

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




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > 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.
> > 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`?
> 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).
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.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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?




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

leonardchan wrote:
> ebevhan wrote:
> > 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.
> 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`?
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).


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

@ebevhan Any more comments on this patch?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 174640.
leonardchan marked an inline comment as done.

Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

ebevhan wrote:
> 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.
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`?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+CommonFixedSema.setSaturated(false);
+

ebevhan wrote:
> Can EmitFixedPointConversion not determine this on its own?
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.



Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // 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

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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 174279.

Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 174235.
leonardchan marked 13 inline comments as done.

Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In https://reviews.llvm.org/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.




Comment at: clang/test/Frontend/fixed_point_add.c:36
+
+  // To smaller scale and same width
+  // CHECK:  [[SA:%[0-9]+]] = load i16, i16* %sa, align 2

ebevhan wrote:
> What do these comments refer to? The scale is the same for both operands here.
This was meant to be a addition with a short _Accum and _Fract, but 
accidentally typed this instead. Added that test after this one.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

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.




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3383
+
+  // onvert the operands to the full precision type.
+  Value *FullLHS = EmitFixedPointConversion(LHS, LHSFixedSema, CommonFixedSema,

Missing a C there.



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

leonardchan wrote:
> ebevhan wrote:
> > Does this handle compound assignment? The other functions handle this 
> > specially.
> 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. 
That's fine by me, then. Take it in the next patch.



Comment at: clang/test/Frontend/fixed_point_add.c:36
+
+  // To smaller scale and same width
+  // CHECK:  [[SA:%[0-9]+]] = load i16, i16* %sa, align 2

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/lib/Basic/FixedPoint.cpp:129
+  std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();

ebevhan wrote:
> 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.
Added test for this also.



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

ebevhan wrote:
> Does this handle compound assignment? The other functions handle this 
> specially.
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. 


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 174080.
leonardchan marked 5 inline comments as done.

Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

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




Comment at: clang/lib/Basic/FixedPoint.cpp:129
+  std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();

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.



Comment at: clang/lib/Basic/FixedPoint.cpp:135
+ResultHasUnsignedPadding =
+hasUnsignedPadding() || Other.hasUnsignedPadding();
+

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



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+CommonFixedSema.setSaturated(false);
+

Can EmitFixedPointConversion not determine this on its own?



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3387
+
+  // Convert and align the operands.
+  Value *LHSAligned = EmitFixedPointConversion(

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



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

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



Comment at: clang/lib/Sema/SemaExpr.cpp:1403
+  else if (RHSType->isFixedPointType())
+return handleFixedPointConversion(*this, RHS, LHS, RHSType, LHSType);
+

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+  if (LHSWidth < CommonWidth)
+LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+  : Builder.CreateZExt(LHS, CommonTy);

bjope wrote:
> `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))
Replaced this part with converting the operands to common semantics, then 
regular addition.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+  if (RHSWidth < CommonWidth)
+RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+  : Builder.CreateZExt(RHS, CommonTy);

bjope wrote:
> Same as above, this can be simplified as:
>   RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);
> 
Same as above.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+  // Align scales
+  unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});

bjope wrote:
> 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.
Same as above.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3223-3247
+if (ResultWidth < CommonWidth) {
+  // In the event we extended the sign of both operands, the intrinsic will
+  // not saturate to the initial bit width of the result type. In this 
case,
+  // we can default to use min/max clamping. This can arise from adding an
+  // operand of an int type whose width is larger than the width of the
+  // other fixed point operand.
+  // If we end up implementing the intrinsics for saturating ints to

leonardchan wrote:
> 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, 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 173961.
leonardchan marked 11 inline comments as done.

Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c
  clang/test/Frontend/fixed_point_conversions.c

Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -214,19 +214,17 @@
   // 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() {
Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1288372, @ebevhan wrote:

> In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
> >
> > > In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
>
> > In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:

> In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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> 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?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1283983, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote:
>
> > In https://reviews.llvm.org/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` 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 

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote:

> In https://reviews.llvm.org/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.)


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1281894, @ebevhan wrote:

> In https://reviews.llvm.org/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?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1280193, @ebevhan wrote:

> In https://reviews.llvm.org/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?


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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.




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3156
+/// the type is an integer, the scale is zero.
+static void GetFixedPointAttributes(ASTContext , QualType Ty,
+unsigned , unsigned ,

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 )` to 
get an FPS for the common full precision semantic between this FPS and another 
one



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3197
+  GetFixedPointAttributes(Ctx, RHSTy, RHSWidth, RHSScale, RHSSign);
+  GetFixedPointAttributes(Ctx, ResultTy, ResultWidth, ResultScale, ResultSign);
+

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.



Comment at: clang/lib/Sema/SemaExpr.cpp:1310
+/// For a given fixed point type, return it's signed equivalent.
+static QualType GetCorrespondingSignedFixedPointType(ASTContext ,
+ QualType Ty) {

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



Comment at: clang/lib/Sema/SemaExpr.cpp:9219
+  else if (RHS.get()->getType()->isFixedPointType())
+compType = FixedPointConversions(RHS, LHS, CompLHSTy);
+  else

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


Repository:
  rC Clang

https://reviews.llvm.org/D53738

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D53738#1278528, @ebevhan wrote:

> In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/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".


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+  if (LHSWidth < CommonWidth)
+LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+  : Builder.CreateZExt(LHS, CommonTy);

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



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+  if (RHSWidth < CommonWidth)
+RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+  : Builder.CreateZExt(RHS, CommonTy);

Same as above, this can be simplified as:
  RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+  // Align scales
+  unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});

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.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3252
+  llvm::Function *intrinsic = CGF.CGM.getIntrinsic(IID, CommonTy);
+  Result = Builder.CreateCall(intrinsic, {LHS, RHS});
+}

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

```


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

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.




Comment at: clang/lib/Sema/SemaExpr.cpp:1393
+/// rules in N1169 4.1.4.
+QualType Sema::FixedPointConversions(ExprResult ,
+ ExprResult , bool IsCompAssign) 
{

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?



Comment at: clang/lib/Sema/SemaExpr.cpp:1416
+  // converted to its corresponding signed fixed-point type and the resulting
+  // type is the type of the converted operand.
+  if (OtherTy->isSignedFixedPointType() &&

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?



Comment at: clang/lib/Sema/SemaExpr.cpp:1435
+  // account rounding and overflow) to the precision of the resulting type.
+  if (OtherTy->isIntegerType())
+return FixedTy;

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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3223-3247
+if (ResultWidth < CommonWidth) {
+  // In the event we extended the sign of both operands, the intrinsic will
+  // not saturate to the initial bit width of the result type. In this 
case,
+  // we can default to use min/max clamping. This can arise from adding an
+  // operand of an int type whose width is larger than the width of the
+  // other fixed point operand.
+  // If we end up implementing the intrinsics for saturating ints to

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. 



Repository:
  rC Clang

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Leonard Chan via Phabricator via cfe-commits]

leonardchan created this revision.
leonardchan added reviewers: ebevhan, bjope, rjmccall.
leonardchan added a project: clang.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D53738

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_add_ast.c

Index: clang/test/Frontend/fixed_point_add_ast.c
===
--- /dev/null
+++ clang/test/Frontend/fixed_point_add_ast.c
@@ -0,0 +1,337 @@
+// RUN: %clang_cc1 -x c -ffixed-point -ast-dump %s | FileCheck %s --strict-whitespace
+
+void SignedAdditions() {
+  // CHECK-LABEL: SignedAdditions
+  short _Accum sa;
+  _Accum a;
+  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;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  sa = sa + sa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  a = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'long _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Accum' {{.*}} 'la' 'long _Accum'
+  la = sa + la;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:   `-ImplicitCastExpr {{.*}} 'long _Accum' 
+  // CHECK-NEXT:`-BinaryOperator {{.*}} '_Accum' '+'
+  // CHECK-NEXT:  |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:  | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:  `-ImplicitCastExpr {{.*}} '_Accum' 
+  // CHECK-NEXT:`-DeclRefExpr {{.*}} '_Accum' {{.*}} 'a' '_Accum'
+  la = sa + a;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Fract' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'short _Fract' {{.*}} 'sf' 'short _Fract'
+  sa = sa + sf;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} '_Fract' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} '_Fract' {{.*}} 'f' '_Fract'
+  sa = sa + f;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'long _Fract' 
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'long _Fract' {{.*}} 'lf' 'long _Fract'
+  sa = sa + lf;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK:  `-BinaryOperator {{.*}} 'short _Accum' '+'
+  // CHECK-NEXT:   |-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT:   | `-DeclRefExpr {{.*}} 'short _Accum' {{.*}} 'sa' 'short _Accum'
+  // CHECK-NEXT:   `-ImplicitCastExpr {{.*}} 'short _Accum' 
+  // CHECK-NEXT: `-ImplicitCastExpr {{.*}} 'unsigned short _Accum' 
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'unsigned short _Accum' {{.*}} 'usa' 'unsigned short _Accum'
+  sa = sa + usa;
+
+  // CHECK-NOT:  FixedPointCast
+  // CHECK: