Page MenuHomePhabricator

[Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation
ClosedPublic

Authored by leonardchan on Dec 18 2018, 4:49 PM.

Diff Detail

Repository
rC Clang

Event Timeline

rjmccall added inline comments.Dec 18 2018, 10:33 PM
clang/include/clang/Basic/FixedPoint.h
98

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?

clang/lib/AST/ExprConstant.cpp
9957

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.

9959

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.)

9963

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.

9986

This unreachable seems more reasonable since you're probably never going to make this a covered switch.

ebevhan added inline comments.Dec 20 2018, 8:21 AM
clang/include/clang/Basic/FixedPoint.h
98

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.

clang/lib/AST/ExprConstant.cpp
1627

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).

9959

Both of those should exist. I think that the FixedPointCast evaluation should only use EvaluateFixedPoint.

9981

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.

clang/lib/Basic/FixedPoint.cpp
49–53

Maybe add a comment here to clarify what we are catching.

clang/test/Frontend/fixed_point_add.c
48

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.

leonardchan marked 13 inline comments as done.

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)
clang/include/clang/Basic/FixedPoint.h
98

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.

clang/lib/AST/ExprConstant.cpp
9957

Yup. Overflow can happen for some non-saturating operations. Added.

9959

I have not yet added either of those but will do so in other patches.

9963

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.

9981

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.

9986

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.

clang/lib/Basic/FixedPoint.cpp
49–53

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.

clang/test/Frontend/fixed_point_add.c
48

Added

ebevhan added inline comments.Jan 15 2019, 8:07 AM
clang/lib/AST/ExprConstant.cpp
7578

This could provide an APFixedPoint rather than APValue.

7592

Technically, this will always produce an APFixedPoint so it doesn't really need to return APValue either.

7594

The semantic is not used in the fixed-point path.

9928–9934

I think I've mentioned this before, but just as a reminder; this doesn't perform correctly for saturation.

9986

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.

clang/lib/Basic/FixedPoint.cpp
25

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.

168

You could use the add_ov functions here.

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

clang/lib/AST/ExprConstant.cpp
9963

Okay.

leonardchan marked 10 inline comments as done.

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

Added D56746 as a child revision of this that has the APValue changes.

clang/lib/AST/ExprConstant.cpp
9928–9934

Added a negate() method to APFixedPoint which handles this now and accounts for saturation.

9986

Done. Double warnings don't seem to appear though.

clang/lib/Basic/FixedPoint.cpp
25

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.

This revision is now accepted and ready to land.Jan 18 2019, 10:01 AM
This revision was automatically updated to reflect the committed changes.