This is an archive of the discontinued LLVM Phabricator instance.

[Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types
Needs ReviewPublic

Authored by leonardchan on May 15 2018, 8:14 PM.

Details

Summary

This patch contains the changes and tests for augmented assignments for primary fixed point types.

s_accum = 7.5hk;
s_accum2 = 2.0hk;
s_accum += s_accum2;
assert(s_accum == 9.5hk);
s_accum += 2.5k;
assert(s_accum == 12);

Logic for moving the radix was also moved to a separate function that takes into account the order between fixed point casts before shifting.

Diff Detail

Repository
rC Clang

Event Timeline

leonardchan created this revision.May 15 2018, 8:14 PM
leonardchan edited the summary of this revision. (Show Details)

Hi @leonardchan.
Could you please prepend some appropriate tag to the subjects of all your differentials, so it is less hard to read through cfe-commits mails, please?

leonardchan retitled this revision from Augmented Assignment for Fixed Point Types to [Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types.May 16 2018, 9:42 AM
Ka-Ka added a subscriber: Ka-Ka.May 23 2018, 1:09 AM
ebevhan added inline comments.
include/clang/AST/Type.h
6562

As I mentioned in another patch, this should be in ASTContext and ask TargetInfo for the information.

I'm not a fan of the term 'fbits'. The correct terminology should be the 'scaling factor'.

lib/CodeGen/CGExprScalar.cpp
1005

For future reference, please do not rely on the behavior of floating point for fixed-point operations.

1181

I think it's safer to simply do all relevant fixed-point conversion by hand here instead of falling through and relying on the semantics of other conversions.

We can provide patches for our implementation here if wanted.

1208

This case should not be needed. Perform all fixed-point conversion before the integer type check above and do an early return instead.

It's not safe to rely on the behavior of other conversions for the behavior of these.