Page MenuHomePhabricator

[Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition
Needs ReviewPublic

Authored by leonardchan on May 16 2018, 3:47 PM.

Details

Summary

This patch contains tests for validating the logic behind each builtin operation on fixed point types and tests on addition between saturated _Fract types.

  • Detecting overflow involves checking if the sign bit flipped or if the first bit in the padding gets flipped.
  • More macros wer added to the FixedPoint.h header on the min and max values for each type which will be required for operations on saturated types.
  • Updated the logic when converting between fixed point types to take into account saturation. Fixed point type conversions do not fall under the "usual arithmetic conversions" where the resulting type on a binary operation resulting in a fixed point type does not need to be the type of either operands.
  • Rounded down _Fract literals of 1 (1.0hr, 1.0r, 1.0lr) to the respective maximum values for each _Fract type.

Diff Detail

Repository
rC Clang

Event Timeline

leonardchan created this revision.May 16 2018, 3:47 PM
leonardchan edited the summary of this revision. (Show Details)May 16 2018, 4:15 PM
  • formatting
  • Running lli threw a segfault in the test, though this was probably because it was using whatever hist jit was available to optimize the code instead of just interpreting it. Forcing it just interpret fixes this.
Ka-Ka added a subscriber: Ka-Ka.May 23 2018, 1:09 AM

I cannot say that I'm pleased with the CodeGen emission of the operations as pure IR. I can only assume that you do not have hardware specifically tailored for these operations, as matching this type of code ought to be quite difficult after optimization is performed.

include/clang/AST/Type.h
6576

If these need to be passed a ASTContext anyway, why not have these functions on ASTContext to begin with?

include/clang/Basic/FixedPoint.h.in
35

As mentioned in other patches, these should not be macros (this entire file should probably be removed altogether).

lib/CodeGen/CGExprScalar.cpp
3111

All of these values can clearly be calculated based on the scaling factor and the width of the type.

3166

Factor out conversion between fixed-point types into its own function so it can be reused for other cases, such as for other operations and actual conversions. It should probably not take QualTypes to convert to but rather arbitrary widths and scales, so it can be used to upscale to/downscale from 'intermediate' common calculation types.

3171

I would much rather see these operations emitted as intrinsics rather than straight IR... but I know that wasn't part of your proposal.

lib/Sema/SemaExpr.cpp
1260

This is incorrect. The resulting type of a binary operation is absolutely one of the two operands. However, the calculation might be done as a type that is not one of the two, as it must be done in the full (combined) precision of both operands.

That is a detail reserved for CodeGen, however.

3541

Should these changes not be part of the patch that adds the literal parsing code?

Also, this patch and all of the following 'Validation Test' patches do much more than just add tests, they add plenty of functionality as well. In general, it's quite difficult to tell which patches add what.

I think it would be much clearer if the patches that claim to add tests only add tests.