[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 <https://reviews.llvm.org/D55868#1358208>, @rjmccall wrote:

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


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