[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC351593: [Fixed Point Arithmetic] Fixed Point Addition 
Constant Expression Evaluation (authored by leonardchan, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55868?vs=181897=182591#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55868

Files:
  include/clang/AST/Expr.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/FixedPoint.h
  lib/AST/ExprConstant.cpp
  lib/Basic/FixedPoint.cpp
  lib/Sema/SemaChecking.cpp
  test/Frontend/fixed_point_add.c
  test/Frontend/fixed_point_conversions.c
  test/Frontend/fixed_point_errors.c

Index: include/clang/AST/Expr.h
===
--- include/clang/AST/Expr.h
+++ include/clang/AST/Expr.h
@@ -23,6 +23,7 @@
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/Basic/CharInfo.h"
+#include "clang/Basic/FixedPoint.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SyncScope.h"
 #include "clang/Basic/TypeTraits.h"
@@ -611,6 +612,12 @@
   EvaluateAsFloat(llvm::APFloat , const ASTContext ,
   SideEffectsKind AllowSideEffects = SE_NoSideEffects) const;
 
+  /// EvaluateAsFloat - Return true if this is a constant which we can fold and
+  /// convert to a fixed point value.
+  bool EvaluateAsFixedPoint(
+  EvalResult , const ASTContext ,
+  SideEffectsKind AllowSideEffects = SE_NoSideEffects) const;
+
   /// isEvaluatable - Call EvaluateAsRValue to see if this expression can be
   /// constant folded without side-effects, but discard the result.
   bool isEvaluatable(const ASTContext ,
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -3190,6 +3190,10 @@
   "implicit truncation from %2 to bit-field changes value from %0 to %1">,
   InGroup;
 
+def warn_impcast_fixed_point_range : Warning<
+  "implicit conversion from %0 cannot fit within the range of values for %1">,
+  InGroup;
+
 def warn_impcast_literal_float_to_integer : Warning<
   "implicit conversion from %0 to %1 changes value from %2 to %3">,
   InGroup;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -61,6 +61,7 @@
 def EnumConversion : DiagGroup<"enum-conversion">;
 def ImplicitIntConversion : DiagGroup<"implicit-int-conversion">;
 def ImplicitFloatConversion : DiagGroup<"implicit-float-conversion">;
+def ImplicitFixedPointConversion : DiagGroup<"implicit-fixed-point-conversion">;
 
 def FloatOverflowConversion : DiagGroup<"float-overflow-conversion">;
 def FloatZeroConversion : DiagGroup<"float-zero-conversion">;
Index: include/clang/Basic/FixedPoint.h
===
--- include/clang/Basic/FixedPoint.h
+++ include/clang/Basic/FixedPoint.h
@@ -118,8 +118,21 @@
 
bool getBoolValue() const { return Val.getBoolValue(); }
 
-   // Convert this number to match the semantics provided.
-   APFixedPoint convert(const FixedPointSemantics ) const;
+   // Convert this number to match the semantics provided. If the overflow
+   // parameter is provided, set this value to true or false to indicate if this
+   // operation results in an overflow.
+   APFixedPoint convert(const FixedPointSemantics ,
+bool *Overflow = nullptr) const;
+
+   // Perform binary operations on a fixed point type. The resulting fixed point
+   // value will be in the common, full precision semantics that can represent
+   // the precision and ranges os both input values. See convert() for an
+   // explanation of the Overflow parameter.
+   APFixedPoint add(const APFixedPoint , bool *Overflow = nullptr) const;
+
+   /// Perform a unary negation (-X) on this fixed point type, taking into
+   /// account saturation if applicable.
+   APFixedPoint negate(bool *Overflow = nullptr) const;
 
APFixedPoint shr(unsigned Amt) const {
  return APFixedPoint(Val >> Amt, Sema);
Index: test/Frontend/fixed_point_errors.c
===
--- test/Frontend/fixed_point_errors.c
+++ test/Frontend/fixed_point_errors.c
@@ -232,3 +232,9 @@
   auto auto_accum = 0k;  // expected-error{{invalid suffix 'k' on integer constant}}
  // expected-warning@-1{{type specifier missing, defaults to 'int'}}
 }
+
+// Overflow
+short _Accum sa_const = 256.0k;   // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'short _Accum'}}
+short _Fract sf_const = 1.0hk;// expected-warning{{implicit conversion from 1.0 

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[John McCall via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

leonardchan updated this revision to Diff 181897.
leonardchan marked 10 inline comments as done.

Repository:
  rC Clang

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

https://reviews.llvm.org/D55868

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_conversions.c
  clang/test/Frontend/fixed_point_errors.c

Index: clang/test/Frontend/fixed_point_errors.c
===
--- clang/test/Frontend/fixed_point_errors.c
+++ clang/test/Frontend/fixed_point_errors.c
@@ -232,3 +232,9 @@
   auto auto_accum = 0k;  // expected-error{{invalid suffix 'k' on integer constant}}
  // expected-warning@-1{{type specifier missing, defaults to 'int'}}
 }
+
+// Overflow
+short _Accum sa_const = 256.0k;   // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'short _Accum'}}
+short _Fract sf_const = 1.0hk;// expected-warning{{implicit conversion from 1.0 cannot fit within the range of values for 'short _Fract'}}
+unsigned _Accum ua_const = -1.0k; // expected-warning{{implicit conversion from -1.0 cannot fit within the range of values for 'unsigned _Accum'}}
+short _Accum sa_const2 = 128.0k + 128.0k; // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'short _Accum'}}
Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -1,5 +1,39 @@
-// RUN: %clang_cc1 -ffixed-point -S -emit-llvm %s -o - | FileCheck %s -check-prefix=DEFAULT
-// RUN: %clang_cc1 -ffixed-point -S -emit-llvm %s -o - -fpadding-on-unsigned-fixed-point | FileCheck %s -check-prefix=SAME
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck %s -check-prefix=DEFAULT
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - -fpadding-on-unsigned-fixed-point | FileCheck %s -check-prefix=SAME
+
+// Between different fixed point types
+short _Accum sa_const = 2.5hk; // DEFAULT-DAG: @sa_const  = {{.*}}global i16 320, align 2
+_Accum a_const = 2.5hk;// DEFAULT-DAG: @a_const   = {{.*}}global i32 81920, align 4
+short _Accum sa_const2 = 2.5k; // DEFAULT-DAG: @sa_const2 = {{.*}}global i16 320, align 2
+
+short _Accum sa_from_f_const = 0.5r; // DEFAULT-DAG: sa_from_f_const = {{.*}}global i16 64, align 2
+_Fract f_from_sa_const = 0.5hk;  // DEFAULT-DAG: f_from_sa_const = {{.*}}global i16 16384, align 2
+
+unsigned short _Accum usa_const = 2.5uk;
+unsigned _Accum ua_const = 2.5uhk;
+// DEFAULT-DAG: @usa_const  = {{.*}}global i16 640, align 2
+// DEFAULT-DAG: @ua_const   = {{.*}}global i32 163840, align 4
+// SAME-DAG:@usa_const  = {{.*}}global i16 320, align 2
+// SAME-DAG:@ua_const   = {{.*}}global i32 81920, align 4
+
+// Signedness
+unsigned short _Accum usa_const2 = 2.5hk;
+// DEFAULT-DAG: @usa_const2  = {{.*}}global i16 640, align 2
+// SAME-DAG:@usa_const2  = {{.*}}global i16 320, align 2
+short _Accum sa_const3 = 2.5hk; // DEFAULT-DAG: @sa_const3 = {{.*}}global i16 320, align 2
+
+// Overflow (this is undefined but allowed)
+short _Accum sa_const4 = 256.0k;
+
+// Saturation
+_Sat short _Accum sat_sa_const = 2.5hk;   // DEFAULT-DAG: @sat_sa_const  = {{.*}}global i16 320, align 2
+_Sat short _Accum sat_sa_const2 = 256.0k; // DEFAULT-DAG: @sat_sa_const2 = {{.*}}global i16 32767, align 2
+_Sat unsigned short _Accum sat_usa_const = -1.0hk;
+// DEFAULT-DAG: @sat_usa_const = {{.*}}global i16 0, align 2
+// SAME-DAG:@sat_usa_const = {{.*}}global i16 0, align 2
+_Sat unsigned short _Accum sat_usa_const2 = 256.0k;
+// DEFAULT-DAG: @sat_usa_const2 = {{.*}}global i16 -1, align 2
+// SAME-DAG:@sat_usa_const2 = {{.*}}global i16 32767, align 2
 
 void TestFixedPointCastSameType() {
   _Accum a = 2.5k;
Index: clang/test/Frontend/fixed_point_add.c
===
--- clang/test/Frontend/fixed_point_add.c
+++ clang/test/Frontend/fixed_point_add.c
@@ -1,5 +1,48 @@
-// 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
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,SIGNED
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -fpadding-on-unsigned-fixed-point -S -emit-llvm %s -o - | FileCheck %s 

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D55868#1358208 , @rjmccall wrote:

> Yeah, I would recommend splitting the `APFixedPoint` in `APValue` changes 
> into a separate patch.


Added D56746  as a child revision of this that 
has the `APValue` changes.




Comment at: clang/lib/AST/ExprConstant.cpp:9927
   }
   return Success(-Value, E);
 }

ebevhan wrote:
> I think I've mentioned this before, but just as a reminder; this doesn't 
> perform correctly for saturation.
Added a `negate()` method to `APFixedPoint` which handles this now and accounts 
for saturation.



Comment at: clang/lib/AST/ExprConstant.cpp:9979
+APFixedPoint Result =
+LHSFX.getFixedPoint().add(RHSFX.getFixedPoint()).convert(ResultFXSema);
+return Success(Result, E);

ebevhan wrote:
> Perhaps this should call HandleOverflow (or do something else) to signal that 
> overflow occurred. HandleOverflow only seems to emit a note, though, but I 
> think it should probably be a warning.
> 
> Maybe for casts as well? Might get double warnings then, though.
Done. Double warnings don't seem to appear though.



Comment at: clang/lib/Basic/FixedPoint.cpp:25
   bool Upscaling = DstScale > getScale();
+  bool Overflowed = false;
 

ebevhan wrote:
> I think it's a bit cleaner if you avoid this variable and simply assign 
> `Overflow` directly here, with the 'else' cases below replaced with `else if 
> (Overflow)`.
> 
> That style isn't cleaner in `add` if you use the APInt add_ov functions 
> though, so maybe it's better to keep it this way for consistency.
I'm not really bothered much by either style, but if we want to be consistent 
with the APInt overflow operations, we could just make the overflow parameter a 
reference also while it's still new/under work.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Yeah, I would recommend splitting the `APFixedPoint` in `APValue` changes into 
a separate patch.




Comment at: clang/lib/AST/ExprConstant.cpp:9959
+  }
+  llvm_unreachable("unknown cast resulting in fixed point value");
+}

leonardchan wrote:
> rjmccall wrote:
> > This is obviously unreachable because of the `default` case in the 
> > `switch`.  Should this be moved into the `default` case, and then you can 
> > put your `Error` call in cases for the known-missing fixed-point 
> > conversions?
> > 
> > You should go ahead and add cases for `CK_NoOp` and `CK_LValueToRValue`, 
> > they should be obvious from the other evaluators.
> > 
> > You should add tests for constant-evaluating fixed-point conversions.
> I think it would be better to keep it as an error since elsewhere, we emit 
> errors indicating a particular operation or conversion has not yet been 
> implemented.
Okay.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:7568
 
+static bool EvaluateFixedPoint(const Expr *E, APValue , EvalInfo ) 
{
+  if (E->getType()->isFixedPointType()) {

This could provide an APFixedPoint rather than APValue.



Comment at: clang/lib/AST/ExprConstant.cpp:7582
+
+static bool EvaluateFixedPointOrInteger(const Expr *E, APValue ,
+EvalInfo ) {

Technically, this will always produce an APFixedPoint so it doesn't really need 
to return APValue either.



Comment at: clang/lib/AST/ExprConstant.cpp:7584
+EvalInfo ) {
+  auto FXSema = Info.Ctx.getFixedPointSemantics(E->getType());
+  if (E->getType()->isIntegerType()) {

The semantic is not used in the fixed-point path.



Comment at: clang/lib/AST/ExprConstant.cpp:9927
   }
   return Success(-Value, E);
 }

I think I've mentioned this before, but just as a reminder; this doesn't 
perform correctly for saturation.



Comment at: clang/lib/AST/ExprConstant.cpp:9979
+APFixedPoint Result =
+LHSFX.getFixedPoint().add(RHSFX.getFixedPoint()).convert(ResultFXSema);
+return Success(Result, E);

Perhaps this should call HandleOverflow (or do something else) to signal that 
overflow occurred. HandleOverflow only seems to emit a note, though, but I 
think it should probably be a warning.

Maybe for casts as well? Might get double warnings then, though.



Comment at: clang/lib/Basic/FixedPoint.cpp:25
   bool Upscaling = DstScale > getScale();
+  bool Overflowed = false;
 

I think it's a bit cleaner if you avoid this variable and simply assign 
`Overflow` directly here, with the 'else' cases below replaced with `else if 
(Overflow)`.

That style isn't cleaner in `add` if you use the APInt add_ov functions though, 
so maybe it's better to keep it this way for consistency.



Comment at: clang/lib/Basic/FixedPoint.cpp:170
+Result = ThisVal + OtherVal;
+Overflowed = ThisVal.isSigned() ? Result.slt(ThisVal) : 
Result.ult(ThisVal);
+  }

You could use the add_ov functions here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

I ended up adding a lot to this patch, so feel free to let me know if I should 
split this up if it's more difficult to review.

Changes:

- Remove default ctors for `APFixedPoint` and `FixedPointSemantics`. The main 
reason I had these was so in the `EvaluateAsFixedPoint*` functions, I could 
pass in an `APFixedPoint` by reference that could be assigned on successful 
evaluation. But it feels more correct now to instead just use an `APValue`.
- Allow `APFixedPoint` to be contained in `APValue`. This takes up a bulk of 
the new changes and could be it's own patch.
- `FixedPointExprEvaluator` now calculates an `APValue` that holds an 
`APFixedPoint`.
- Removed unused `Success` methods in `FixedPointExprEvaluator`.
- Added `EvaluateAsFixedPoint` functions and method to `Expr`.
- Added tests for constant-evaluating fixed point conversions.
- Added warning for potential overflow conversions when assigning to fixed 
point types (as a part of adding tests for conversions)
- `APFixedPoint` `convert()` and `add()` methods take an optional second 
argument for indicating if the operation resulted in an overflow (although this 
could be changed for a clearer way to check for operator overflow)




Comment at: clang/include/clang/Basic/FixedPoint.h:98
  public:
+   APFixedPoint() = default;
APFixedPoint(const llvm::APInt , const FixedPointSemantics )

ebevhan wrote:
> rjmccall wrote:
> > This should be documented to describe what it does.  Does it create a value 
> > with impossible semantics?  Some reasonable default value in some 
> > reasonable default semantics?
> As it is, it doesn't seem to create anything useful; neither does the one for 
> the `FixedPointSemantics`. It would be a zero-width, zero-scale value.
The idea behind this was to enable a value to later be reassigned to an 
APFixedPoint, similar to how APValues are assigned in `EvaluateInteger` on 
success. I suppose the correct thing to do in this case would be to instead 
allow for APValue to take an APFixedPoint instead and pass that to 
`EvaluateFixedPoint*` to make it consistent with the other Evaluate methods and 
allow for APFixedPoint to be initialized with a default value.



Comment at: clang/lib/AST/ExprConstant.cpp:9953
+  return false;
+APFixedPoint Result = Src.convert(DestFXSema);
+return Success(Result, E);

rjmccall wrote:
> Can a fixed-point conversion ever have undefined behavior?  If so, you might 
> need to flag that as a failed case, unless we want to give it defined 
> semantics in Clang.
Yup. Overflow can happen for some non-saturating operations. Added.



Comment at: clang/lib/AST/ExprConstant.cpp:9955
+return Success(Result, E);
+  }
+  default:

ebevhan wrote:
> rjmccall wrote:
> > Do you not have a separate `CK_IntegralToFixedPoint`?  It's convenient 
> > here, but still, it's curious.
> > 
> > You also need a `CK_FloatingToFixedPoint`, I think.  (Obviously you don't 
> > need to implement that right now.)
> Both of those should exist. I think that the `FixedPointCast` evaluation 
> should only use `EvaluateFixedPoint`.
I have not yet added either of those but will do so in other patches.



Comment at: clang/lib/AST/ExprConstant.cpp:9959
+  }
+  llvm_unreachable("unknown cast resulting in fixed point value");
+}

rjmccall wrote:
> This is obviously unreachable because of the `default` case in the `switch`.  
> Should this be moved into the `default` case, and then you can put your 
> `Error` call in cases for the known-missing fixed-point conversions?
> 
> You should go ahead and add cases for `CK_NoOp` and `CK_LValueToRValue`, they 
> should be obvious from the other evaluators.
> 
> You should add tests for constant-evaluating fixed-point conversions.
I think it would be better to keep it as an error since elsewhere, we emit 
errors indicating a particular operation or conversion has not yet been 
implemented.



Comment at: clang/lib/AST/ExprConstant.cpp:9977
+APFixedPoint Result = LHSFX.add(RHSFX).convert(ResultFXSema);
+return Success(Result, E);
+  }

ebevhan wrote:
> I understand the benefit of placing the actual operation implementation in 
> the `APFixedPoint` class, but it means that any intermediate information is 
> sort of lost. For example, if we want to warn the user about overflow (into 
> the padding bit, or just in general) we can't, because that information was 
> hidden.
> 
> I guess it could be done by checking if the result of the `add` is equal to 
> the result of the `convert` for non-saturating `ResultFXSema`? The 
> `APFixedPoint` comparison is value-based. Not sure how it would work with the 
> padding bit in unsigned types, though.
We could do something like having a separate method that checks for overflow or 
like `bool addCanOverflow(APFixedPoint LHS, APFixedPoint RHS)`, or an 

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

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

Repository:
  rC Clang

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

https://reviews.llvm.org/D55868

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/Type.h
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/Type.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/test/Frontend/fixed_point_add.c
  clang/test/Frontend/fixed_point_conversions.c
  clang/test/Frontend/fixed_point_errors.c

Index: clang/test/Frontend/fixed_point_errors.c
===
--- clang/test/Frontend/fixed_point_errors.c
+++ clang/test/Frontend/fixed_point_errors.c
@@ -232,3 +232,8 @@
   auto auto_accum = 0k;  // expected-error{{invalid suffix 'k' on integer constant}}
  // expected-warning@-1{{type specifier missing, defaults to 'int'}}
 }
+
+// Overflow
+short _Accum sa_const = 256.0k;   // expected-warning{{implicit conversion from 256.0 cannot fit within the range of values for 'short _Accum'}}
+short _Fract sf_const = 1.0hk;// expected-warning{{implicit conversion from 1.0 cannot fit within the range of values for 'short _Fract'}}
+unsigned _Accum ua_const = -1.0k; // expected-warning{{implicit conversion from -1.0 cannot fit within the range of values for 'unsigned _Accum'}}
Index: clang/test/Frontend/fixed_point_conversions.c
===
--- clang/test/Frontend/fixed_point_conversions.c
+++ clang/test/Frontend/fixed_point_conversions.c
@@ -1,5 +1,39 @@
-// RUN: %clang_cc1 -ffixed-point -S -emit-llvm %s -o - | FileCheck %s -check-prefix=DEFAULT
-// RUN: %clang_cc1 -ffixed-point -S -emit-llvm %s -o - -fpadding-on-unsigned-fixed-point | FileCheck %s -check-prefix=SAME
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck %s -check-prefix=DEFAULT
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - -fpadding-on-unsigned-fixed-point | FileCheck %s -check-prefix=SAME
+
+// Between different fixed point types
+short _Accum sa_const = 2.5hk; // DEFAULT-DAG: @sa_const  = {{.*}}global i16 320, align 2
+_Accum a_const = 2.5hk;// DEFAULT-DAG: @a_const   = {{.*}}global i32 81920, align 4
+short _Accum sa_const2 = 2.5k; // DEFAULT-DAG: @sa_const2 = {{.*}}global i16 320, align 2
+
+short _Accum sa_from_f_const = 0.5r; // DEFAULT-DAG: sa_from_f_const = {{.*}}global i16 64, align 2
+_Fract f_from_sa_const = 0.5hk;  // DEFAULT-DAG: f_from_sa_const = {{.*}}global i16 16384, align 2
+
+unsigned short _Accum usa_const = 2.5uk;
+unsigned _Accum ua_const = 2.5uhk;
+// DEFAULT-DAG: @usa_const  = {{.*}}global i16 640, align 2
+// DEFAULT-DAG: @ua_const   = {{.*}}global i32 163840, align 4
+// SAME-DAG:@usa_const  = {{.*}}global i16 320, align 2
+// SAME-DAG:@ua_const   = {{.*}}global i32 81920, align 4
+
+// Signedness
+unsigned short _Accum usa_const2 = 2.5hk;
+// DEFAULT-DAG: @usa_const2  = {{.*}}global i16 640, align 2
+// SAME-DAG:@usa_const2  = {{.*}}global i16 320, align 2
+short _Accum sa_const3 = 2.5hk; // DEFAULT-DAG: @sa_const3 = {{.*}}global i16 320, align 2
+
+// Overflow (this is undefined but allowed)
+short _Accum sa_const4 = 256.0k;
+
+// Saturation
+_Sat short _Accum sat_sa_const = 2.5hk;   // DEFAULT-DAG: @sat_sa_const  = {{.*}}global i16 320, align 2
+_Sat short _Accum sat_sa_const2 = 256.0k; // DEFAULT-DAG: @sat_sa_const2 = {{.*}}global i16 32767, align 2
+_Sat unsigned short _Accum sat_usa_const = -1.0hk;
+// DEFAULT-DAG: @sat_usa_const = {{.*}}global i16 0, align 2
+// SAME-DAG:@sat_usa_const = {{.*}}global i16 0, align 2
+_Sat unsigned short _Accum sat_usa_const2 = 256.0k;
+// DEFAULT-DAG: @sat_usa_const2 = {{.*}}global i16 -1, align 2
+// SAME-DAG:@sat_usa_const2 = {{.*}}global i16 32767, align 2
 
 void TestFixedPointCastSameType() {
   _Accum a = 2.5k;
Index: clang/test/Frontend/fixed_point_add.c
===
--- clang/test/Frontend/fixed_point_add.c
+++ clang/test/Frontend/fixed_point_add.c
@@ -1,5 +1,48 @@
-// 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
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S -emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,SIGNED
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -fpadding-on-unsigned-fixed-point -S -emit-llvm %s -o - | 

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/include/clang/Basic/FixedPoint.h:98
  public:
+   APFixedPoint() = default;
APFixedPoint(const llvm::APInt , const FixedPointSemantics )

rjmccall wrote:
> This should be documented to describe what it does.  Does it create a value 
> with impossible semantics?  Some reasonable default value in some reasonable 
> default semantics?
As it is, it doesn't seem to create anything useful; neither does the one for 
the `FixedPointSemantics`. It would be a zero-width, zero-scale value.



Comment at: clang/lib/AST/ExprConstant.cpp:1618
+/// Evaluate an integer or fixed point expression into an APFixedPoint.
+static bool EvaluateFixedPointOrInteger(const Expr *E, APFixedPoint ,
+EvalInfo );

I think I would like to see both `EvaluateFixedPoint` and 
`EvaluateFixedPointOrInteger`, and use the former when you know it's supposed 
to be a fixed-point value (like in `FixedPointCast`).



Comment at: clang/lib/AST/ExprConstant.cpp:9955
+return Success(Result, E);
+  }
+  default:

rjmccall wrote:
> Do you not have a separate `CK_IntegralToFixedPoint`?  It's convenient here, 
> but still, it's curious.
> 
> You also need a `CK_FloatingToFixedPoint`, I think.  (Obviously you don't 
> need to implement that right now.)
Both of those should exist. I think that the `FixedPointCast` evaluation should 
only use `EvaluateFixedPoint`.



Comment at: clang/lib/AST/ExprConstant.cpp:9977
+APFixedPoint Result = LHSFX.add(RHSFX).convert(ResultFXSema);
+return Success(Result, E);
+  }

I understand the benefit of placing the actual operation implementation in the 
`APFixedPoint` class, but it means that any intermediate information is sort of 
lost. For example, if we want to warn the user about overflow (into the padding 
bit, or just in general) we can't, because that information was hidden.

I guess it could be done by checking if the result of the `add` is equal to the 
result of the `convert` for non-saturating `ResultFXSema`? The `APFixedPoint` 
comparison is value-based. Not sure how it would work with the padding bit in 
unsigned types, though.



Comment at: clang/lib/Basic/FixedPoint.cpp:42
 
-if (!DstSema.isSigned() && NewVal.isNegative())
+if (!DstSema.isSigned() && NewVal.isSigned() && NewVal.isNegative())
   NewVal = 0;

Maybe add a comment here to clarify what we are catching.



Comment at: clang/test/Frontend/fixed_point_add.c:5
+// Addition between different fixed point types
+short _Accum sa_const = 1.0hk + 2.0hk;  // CHECK-DAG: @sa_const  = 
{{.*}}global i16 384, align 2
+_Accum a_const = 1.0hk + 2.0k;  // CHECK-DAG: @a_const   = 
{{.*}}global i32 98304, align 4

The test doesn't have a triple so the alignment might be affected by what 
machine the test runs on.

Now that I think about it, so will the results of the calculations, and the 
emitted IR in the functions.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/FixedPoint.h:98
  public:
+   APFixedPoint() = default;
APFixedPoint(const llvm::APInt , const FixedPointSemantics )

This should be documented to describe what it does.  Does it create a value 
with impossible semantics?  Some reasonable default value in some reasonable 
default semantics?



Comment at: clang/lib/AST/ExprConstant.cpp:9953
+  return false;
+APFixedPoint Result = Src.convert(DestFXSema);
+return Success(Result, E);

Can a fixed-point conversion ever have undefined behavior?  If so, you might 
need to flag that as a failed case, unless we want to give it defined semantics 
in Clang.



Comment at: clang/lib/AST/ExprConstant.cpp:9955
+return Success(Result, E);
+  }
+  default:

Do you not have a separate `CK_IntegralToFixedPoint`?  It's convenient here, 
but still, it's curious.

You also need a `CK_FloatingToFixedPoint`, I think.  (Obviously you don't need 
to implement that right now.)



Comment at: clang/lib/AST/ExprConstant.cpp:9959
+  }
+  llvm_unreachable("unknown cast resulting in fixed point value");
+}

This is obviously unreachable because of the `default` case in the `switch`.  
Should this be moved into the `default` case, and then you can put your `Error` 
call in cases for the known-missing fixed-point conversions?

You should go ahead and add cases for `CK_NoOp` and `CK_LValueToRValue`, they 
should be obvious from the other evaluators.

You should add tests for constant-evaluating fixed-point conversions.



Comment at: clang/lib/AST/ExprConstant.cpp:9982
+  }
+  llvm_unreachable("Should've exited before this");
+}

This `unreachable` seems more reasonable since you're probably never going to 
make this a covered `switch`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868



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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Leonard Chan via Phabricator via cfe-commits]

leonardchan created this revision.
leonardchan added reviewers: ebevhan, bjope, rjmccall.
leonardchan added a project: clang.
leonardchan added a parent revision: D53738: [Fixed Point Arithmetic] Fixed 
Point Addition.

This patch includes logic for constant expression evaluation of fixed point 
additions.


Repository:
  rC Clang

https://reviews.llvm.org/D55868

Files:
  clang/include/clang/Basic/FixedPoint.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Basic/FixedPoint.cpp
  clang/test/Frontend/fixed_point_add.c

Index: clang/test/Frontend/fixed_point_add.c
===
--- clang/test/Frontend/fixed_point_add.c
+++ clang/test/Frontend/fixed_point_add.c
@@ -1,6 +1,49 @@
 // 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
 
+// Addition between different fixed point types
+short _Accum sa_const = 1.0hk + 2.0hk;  // CHECK-DAG: @sa_const  = {{.*}}global i16 384, align 2
+_Accum a_const = 1.0hk + 2.0k;  // CHECK-DAG: @a_const   = {{.*}}global i32 98304, align 4
+long _Accum la_const = 1.0hk + 2.0lk;   // CHECK-DAG: @la_const  = {{.*}}global i64 6442450944, align 8
+short _Accum sa_const2 = 0.5hr + 2.0hk; // CHECK-DAG: @sa_const2  = {{.*}}global i16 320, align 2
+short _Accum sa_const3 = 0.5r + 2.0hk;  // CHECK-DAG: @sa_const3  = {{.*}}global i16 320, align 2
+short _Accum sa_const4 = 0.5lr + 2.0hk; // CHECK-DAG: @sa_const4  = {{.*}}global i16 320, align 2
+
+// Unsigned addition
+unsigned short _Accum usa_const = 1.0uhk + 2.0uhk;
+// CHECK-SIGNED-DAG:   @usa_const = {{.*}}global i16 768, align 2
+// CHECK-UNSIGNED-DAG: @usa_const = {{.*}}global i16 384, align 2
+
+// Unsigned + signed
+short _Accum sa_const5 = 1.0uhk + 2.0hk;
+// CHECK-DAG: @sa_const5 = {{.*}}global i16 384, align 2
+
+// Addition with negative number
+short _Accum sa_const6 = 0.5hr + (-2.0hk);
+// CHECK-DAG: @sa_const6 = {{.*}}global i16 -192, align 2
+
+// Int addition
+unsigned short _Accum usa_const2 = 2 + 0.5uhk;
+// CHECK-SIGNED-DAG:   @usa_const2 = {{.*}}global i16 640, align 2
+// CHECK-UNSIGNED-DAG: @usa_const2 = {{.*}}global i16 320, align 2
+short _Accum sa_const7 = 2 + (-0.5hk);   // CHECK-DAG: @sa_const7 = {{.*}}global i16 192, align 2
+short _Accum sa_const8 = 257 + (-2.0hk); // CHECK-DAG: @sa_const8 = {{.*}}global i16 32640, align 2
+long _Fract lf_const = -0.5lr + 1;   // CHECK-DAG: @lf_const  = {{.*}}global i32 1073741824, align 4
+
+// Saturated addition
+_Sat short _Accum sat_sa_const = (_Sat short _Accum)128.0hk + 128.0hk;
+// CHECK-DAG: @sat_sa_const = {{.*}}global i16 32767, align 2
+_Sat unsigned short _Accum sat_usa_const = (_Sat unsigned short _Accum)128.0uhk + 128.0uhk;
+// CHECK-SIGNED-DAG:   @sat_usa_const = {{.*}}global i16 65535, align 2
+// CHECK-UNSIGNED-DAG: @sat_usa_const = {{.*}}global i16 32767, align 2
+_Sat short _Accum sat_sa_const2 = (_Sat short _Accum)128.0hk + 128;
+// CHECK-DAG: @sat_sa_const2 = {{.*}}global i16 32767, align 2
+_Sat unsigned short _Accum sat_usa_const2 = (_Sat unsigned short _Accum)128.0uhk + 128;
+// CHECK-SIGNED-DAG:   @sat_usa_const2 = {{.*}}global i16 65535, align 2
+// CHECK-UNSIGNED-DAG: @sat_usa_const2 = {{.*}}global i16 32767, align 2
+_Sat unsigned short _Accum sat_usa_const3 = (_Sat unsigned short _Accum)0.5uhk + (-2);
+// CHECK-DAG:   @sat_usa_const3 = {{.*}}global i16 0, align 2
+
 void SignedAddition() {
   // CHECK-LABEL: SignedAddition
   short _Accum sa;
Index: clang/lib/Basic/FixedPoint.cpp
===
--- clang/lib/Basic/FixedPoint.cpp
+++ clang/lib/Basic/FixedPoint.cpp
@@ -39,7 +39,7 @@
 if (!(Masked == Mask || Masked == 0))
   NewVal = NewVal.isNegative() ? Mask : ~Mask;
 
-if (!DstSema.isSigned() && NewVal.isNegative())
+if (!DstSema.isSigned() && NewVal.isSigned() && NewVal.isNegative())
   NewVal = 0;
   }
 
@@ -137,4 +137,21 @@
  ResultIsSaturated, ResultHasUnsignedPadding);
 }
 
+APFixedPoint APFixedPoint::add(const APFixedPoint ) const {
+  auto CommonFXSema = Sema.getCommonSemantics(Other.getSemantics());
+  APFixedPoint ConvertedThis = convert(CommonFXSema);
+  APFixedPoint ConvertedOther = Other.convert(CommonFXSema);
+  llvm::APSInt ThisVal = ConvertedThis.getValue();
+  llvm::APSInt OtherVal = ConvertedOther.getValue();
+
+  llvm::APInt Result;
+  if (CommonFXSema.isSaturated()) {
+Result = CommonFXSema.isSigned() ? ThisVal.sadd_sat(OtherVal)
+ : ThisVal.uadd_sat(OtherVal);
+  } else
+Result = ThisVal + OtherVal;
+
+  return APFixedPoint(Result, CommonFXSema);
+}
+
 }  // namespace clang
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++