[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 &Val, const FixedPointSemantics &Sema)
----------------
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 &Result,
+                                        EvalInfo &Info);
----------------
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