This patch includes logic for constant expression evaluation of fixed point additions.
ebevhan bjope rjmccall
- rL351593: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation
rGd3f3e16293e5: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation
rC351593: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation
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?
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.
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.)
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.
This unreachable seems more reasonable since you're probably never going to make this a covered switch.
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.
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).
Both of those should exist. I think that the FixedPointCast evaluation should only use EvaluateFixedPoint.
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.
Maybe add a comment here to clarify what we are catching.
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.
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.
- 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)
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.
Yup. Overflow can happen for some non-saturating operations. Added.
I have not yet added either of those but will do so in other patches.
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.
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.
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.
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.
This could provide an APFixedPoint rather than APValue.
Technically, this will always produce an APFixedPoint so it doesn't really need to return APValue either.
The semantic is not used in the fixed-point path.
I think I've mentioned this before, but just as a reminder; this doesn't perform correctly for saturation.
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.
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.
You could use the add_ov functions here.
Added D56746 as a child revision of this that has the APValue changes.
Added a negate() method to APFixedPoint which handles this now and accounts for saturation.
Done. Double warnings don't seem to appear though.
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.