[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 &Result, EvalInfo &Info) {
+  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 &Result,
+                                        EvalInfo &Info) {
----------------
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 &Info) {
+  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