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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
