[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 &Val, const FixedPointSemantics &Sema)
----------------
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 additional optional pointer argument to the add method like `APFixedPoint add(const APFixedPoint &Other, bool *overflow=nullptr)` where if `overflow` is not a nullptr, we would still perform the addition but set this to indicate if we overflowed or not. 

For now I went with the second option since this is useful now for checking overflow on casts and addition, but this could also be discussed/changed later.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9982
+  }
+  llvm_unreachable("Should've exited before this");
+}
----------------
rjmccall wrote:
> This `unreachable` seems more reasonable since you're probably never going to make this a covered `switch`.
Should I just remove these? I recall seeing some other switch statements in clang where after all cases are covered, there's an llvm_unreachable and I only put this here for consistency.


================
Comment at: clang/lib/Basic/FixedPoint.cpp:42
 
-    if (!DstSema.isSigned() && NewVal.isNegative())
+    if (!DstSema.isSigned() && NewVal.isSigned() && NewVal.isNegative())
       NewVal = 0;
----------------
ebevhan wrote:
> Maybe add a comment here to clarify what we are catching.
Added. I didn't know this before, but it turns out that `isNegative()` only checks if the last bit is set, but not if the value is signed or not.


================
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
----------------
ebevhan wrote:
> 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.
Added


Repository:
  rC Clang

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

https://reviews.llvm.org/D55868