bricci added a comment.
Just to provide a bit of additional context:
My comment about fsyntax-only is not specific to fsyntax-only.
This is just an handy way to benchmark the frontend without noise
from LLVM. It is important to keep all frequently used structures small
(within reason) even if
bricci added a comment.
Added some comments inline.
However I have not looked at the logic too closely and
have not looked at the tests at all.
Comment at: cfe/trunk/include/clang/AST/ASTContext.h:1966
unsigned char getFixedPointIBits(QualType Ty) const;
+
leonardchan added a comment.
In https://reviews.llvm.org/D48661#1189537, @bricci wrote:
> Just a nit but could you please add new-lines to your commit messages.
My bad, will remember this for future commits.
> Also the 3 bools in FixedPointSemantics are a little bit wasteful.
> I don't know
bricci added a comment.
And using the name Sema is a bad idea IMHO since this has a totally different
meaning in clang.
Repository:
rL LLVM
https://reviews.llvm.org/D48661
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
bricci added a comment.
Just a nit but could you please add new-lines to your commit messages.
Also the 3 bools in FixedPointSemantics are a little bit wasteful.
Repository:
rL LLVM
https://reviews.llvm.org/D48661
___
cfe-commits mailing list
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL339028: [Fixed Point Arithmetic] Fixed Point Constant
(authored by leonardchan, committed by ).
Herald added a
leonardchan updated this revision to Diff 159320.
leonardchan marked an inline comment as done.
leonardchan added a comment.
- Fixed `Accumum` names
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/FixedPoint.h
ebevhan added a comment.
Sorry for the late response, I've been away.
LGTM, although my browser doesn't want to let me set Accepted on this.
Comment at: unittests/Basic/FixedPointTest.cpp:380
+}
+
+// Check the value in a given fixed point sema overflows to the saturated max
leonardchan updated this revision to Diff 156280.
leonardchan marked an inline comment as done.
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/FixedPoint.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
rjmccall added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:64
+
+class APFixedPoint {
+ public:
rjmccall wrote:
> This should get a documentation comment describing the class. You should
> mention that, like `APSInt`, it carries all of the
leonardchan updated this revision to Diff 156174.
leonardchan marked 3 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/FixedPoint.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:1953
unsigned char getFixedPointIBits(QualType Ty) const;
+ FixedPointSemantics getFixedPointSema(QualType Ty) const;
+ APFixedPoint getFixedPointMax(QualType Ty) const;
rjmccall
leonardchan added a comment.
@ebevhan Any followup on the patch/my previous comments?
Repository:
rC Clang
https://reviews.llvm.org/D48661
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
leonardchan added inline comments.
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+Val = Val.extend(DstWidth);
+ if (Upscaling)
ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > It should be possible to replace this
leonardchan updated this revision to Diff 154465.
leonardchan marked 4 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/FixedPoint.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
ebevhan added inline comments.
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+Val = Val.extend(DstWidth);
+ if (Upscaling)
leonardchan wrote:
> ebevhan wrote:
> > It should be possible to replace this with `extOrTrunc` and
rjmccall added a comment.
Thanks, much appreciated. A couple more style changes that I noticed and this
will be LGTM, although you should also make sure you have Bevin Hansson's
approval.
Comment at: include/clang/AST/ASTContext.h:33
#include "clang/Basic/AddressSpaces.h"
leonardchan updated this revision to Diff 154020.
leonardchan marked 2 inline comments as done.
leonardchan added a comment.
- Renamed `fixedPointSemantics` to `FixedPointSemantics` and hid the members
behind getters
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
rjmccall added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:31
+ SatNoPadding,
+};
+
leonardchan wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > I figured you'd want this to be a struct which include the scale, width,
> > > signed-ness, and
leonardchan added inline comments.
Comment at: lib/Basic/FixedPoint.cpp:40
+ if (DstWidth > Val.getBitWidth())
+Val = Val.extend(DstWidth);
+ if (Upscaling)
ebevhan wrote:
> It should be possible to replace this with `extOrTrunc` and move it below the
>
leonardchan updated this revision to Diff 153924.
leonardchan marked 7 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D48661
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/FixedPoint.h
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
ebevhan added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:67
+ // Convert this number to match the semantics provided.
+ void convert(const struct fixedPointSemantics );
+
If this class is supposed to be used like APInt and APSInt, perhaps
leonardchan added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:31
+ SatNoPadding,
+};
+
ebevhan wrote:
> rjmccall wrote:
> > I figured you'd want this to be a struct which include the scale, width,
> > signed-ness, and saturating-ness; and
leonardchan updated this revision to Diff 153821.
leonardchan marked 10 inline comments as done.
leonardchan edited the summary of this revision.
leonardchan added a comment.
- Added tests
- Moved all conversion logic into `convert`
- Saturation is checked by checking the bits above the sign bit
ebevhan added inline comments.
Comment at: lib/Basic/FixedPoint.cpp:53
+// We can overflow here
+unsigned ShiftAmt = DstScale - Scale;
+if (Val < 0 && Val.countLeadingOnes() >= ShiftAmt)
ebevhan wrote:
> I think saturation can be modeled a bit
ebevhan added a comment.
I also think it would be good with some unit tests for this class once the
functionality and interface is nailed down.
Comment at: include/clang/Basic/FixedPoint.h:31
+ SatNoPadding,
+};
+
rjmccall wrote:
> I figured you'd want this
rjmccall added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:31
+ SatNoPadding,
+};
+
I figured you'd want this to be a struct which include the scale, width,
signed-ness, and saturating-ness; and then `APFixedPoint` can just store one of
leonardchan updated this revision to Diff 153426.
leonardchan marked 8 inline comments as done.
leonardchan added a comment.
Herald added a subscriber: mgorny.
- Renamed to APFixedPoint
- Added `FixedPointSemantics` to represent saturation and whether or not
padding is involved. Similar to
leonardchan added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
rjmccall wrote:
> ebevhan wrote:
> > rjmccall wrote:
> > > The established naming convention here — as seen in `APInt`, `APFloat`,
> > >
rjmccall added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
ebevhan wrote:
> rjmccall wrote:
> > The established naming convention here — as seen in `APInt`, `APFloat`,
> > `APValue`, etc. — would call
ebevhan added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:23
+
+class FixedPointNumber {
+ public:
rjmccall wrote:
> The established naming convention here — as seen in `APInt`, `APFloat`,
> `APValue`, etc. — would call this `APFixedPoint`.
rjmccall added inline comments.
Comment at: include/clang/Basic/FixedPoint.h:11
+/// \file
+/// Defines the fixed point number interface.
+//
Referencing the standard here would be good, and maybe even excerpting the key
parts.
Comment at:
leonardchan created this revision.
leonardchan added reviewers: phosek, mcgrathr, jakehehrlich, rsmith, ebevhan,
rjmccall.
leonardchan added a project: clang.
This patch proposes an abstract type that represents fixed point numbers,
similar to APInt or APSInt that was discussed in
33 matches
Mail list logo
