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 thi
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;
+ FixedPointSemant
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 h
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
http://lists.llvm
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
cfe
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 subscriber:
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
include/clang
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
lib/Basi
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 sem
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
lib/Basi
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 wrot
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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
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 w
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
lib/Basi
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 move
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:
include/clan
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 s
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
> sa
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
lib/Basi
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 &DstSema);
+
If this class is supposed to be used like APInt and APSInt, per
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 then
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 i
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 better.
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 t
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
these
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 `APFlo
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`,
> > > `APV
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 th
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`. Ma
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: inc
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
https://revie
33 matches
Mail list logo
