jlebar added inline comments.
Comment at: llvm/unittests/ADT/APFloatTest.cpp:3534
Op2[1])
.str();
}
timshen wrote:
> jlebar wrote:
> > Honestly I am not sure this is an improvement over writing it out by hand:
> >
> >
timshen marked 13 inline comments as done.
timshen added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:1054
+ opStatus next(bool nextDown) {
+if (usesLayout(getSemantics()))
jlebar wrote:
> FWIW, see my screed on this:
>
jlebar added a comment.
Eric asked me to do a review of this. I notice now it was submitted while I
was doing the review. Oh well. Here are the comments I had.
Comment at: llvm/include/llvm/ADT/APFloat.h:1054
+ opStatus next(bool nextDown) {
+if
This revision was automatically updated to reflect the committed changes.
Closed by commit rL292839: [APFloat] Switch from (PPCDoubleDoubleImpl,
IEEEdouble) layout to (IEEEdouble… (authored by timshen).
Changed prior to commit:
https://reviews.llvm.org/D27872?vs=85217=85457#toc
Repository:
echristo accepted this revision.
echristo added a comment.
This revision is now accepted and ready to land.
This looks fine to me now, might be good to get someone else to ack as well
though.
https://reviews.llvm.org/D27872
___
cfe-commits mailing
timshen added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:1039
+ /// \brief Operator+ overload which provides the default
+ /// \c nmNearestTiesToEven rounding mode and *no* error checking.
APFloat operator+(const APFloat ) const {
echristo
timshen updated this revision to Diff 85217.
timshen marked 2 inline comments as done.
timshen added a comment.
Stylish changes.
https://reviews.llvm.org/D27872
Files:
clang/test/CodeGen/ppc64-complex-parms.c
llvm/include/llvm/ADT/APFloat.h
llvm/lib/Support/APFloat.cpp
timshen marked an inline comment as done.
timshen added a comment.
Friendly ping. :)
We still have internal test failures that this patch (and the next one) fixes,
and I think this is the last "hard to review" patch in the APFloat refactoring.
https://reviews.llvm.org/D27872
timshen marked an inline comment as done.
timshen added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:791
void makeNaN(bool SNaN, bool Neg, const APInt *fill) {
-getIEEE().makeNaN(SNaN, Neg, fill);
+if (usesLayout(getSemantics()))
+ return
timshen added a comment.
In https://reviews.llvm.org/D27872#639020, @hfinkel wrote:
> it is not at all obvious what is going on (i.e. why are we casting to
> integers?). Maybe semPPCDoubleDoubleImpl needs a better name now? It seems
> confusing to have semPPCDoubleDoubleImpl and
timshen updated this revision to Diff 83700.
timshen added a comment.
Rename semPPCDoubleDoubleImpl to semPPCDoubleDoubleLegacy to reflect its use
more accurately.
https://reviews.llvm.org/D27872
Files:
clang/test/CodeGen/ppc64-complex-parms.c
llvm/include/llvm/ADT/APFloat.h
hfinkel added a comment.
> (IEEEdouble, IEEEdouble) -> (uint64_t, uint64_t) -> PPCDoubleDoubleImpl, then
> run the old algorithm.
We need to document in the code what is going on here and why it works. Just
looking at a bunch of code like this:
if (Semantics == ) {
APFloat
echristo added a reviewer: scanon.
echristo added a comment.
Adding Steve in an attempt to get him to review :)
https://reviews.llvm.org/D27872
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
timshen added a comment.
Friendly ping :)
Is there anyone else I can add as a reviewer, if no one feels comfortable
reviewing the semantic?
https://reviews.llvm.org/D27872
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
timshen added a comment.
In https://reviews.llvm.org/D27872#636149, @echristo wrote:
> I'm pretty sure I've never seen return widely used in
> the code base versus my suggestion. That said, if you've looked and it's
> roughly 50/50 then I care a lot less (and we can bike shed in some separate
echristo added a comment.
In https://reviews.llvm.org/D27872#636147, @timshen wrote:
> In https://reviews.llvm.org/D27872#636130, @echristo wrote:
>
> > Looks pretty weird. Typically I'd suggest just:
> >
> > if (foo) {
> >
> > Foo();
> > return;
> >
> > }
> >
> > since that will keep
timshen added a comment.
In https://reviews.llvm.org/D27872#636130, @echristo wrote:
> Looks pretty weird. Typically I'd suggest just:
>
> if (foo) {
>
> Foo();
> return;
>
> }
>
> since that will keep cognitive overhead to a minimum.
>
> -eric
>
> > Other functions are not controversial.
echristo added a comment.
In https://reviews.llvm.org/D27872#628212, @timshen wrote:
> I changed type style to early return.
>
> For constructors and destructors, I use:
>
> if (...) {
> // statement;
> return;
> }
>
>
> For normal functions that returns void, I chose:
>
> if
timshen added a comment.
I changed type style to early return.
For constructors and destructors, I use:
if (...) {
// statement;
return;
}
For normal functions that returns void, I chose:
if (...)
return Foo();
llvm_unreachable(...);
since it's more compact. If returning
timshen updated this revision to Diff 82148.
timshen added a comment.
Consistently use early return style.
https://reviews.llvm.org/D27872
Files:
clang/test/CodeGen/ppc64-complex-parms.c
llvm/include/llvm/ADT/APFloat.h
llvm/lib/Support/APFloat.cpp
mehdi_amini added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:800
- void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); }
+ void makeLargest(bool Neg) {
+if (usesLayout(getSemantics())) {
jtony wrote:
> I know it is allowed to
timshen updated this revision to Diff 81981.
timshen added a comment.
Remove more 'else' after return.
https://reviews.llvm.org/D27872
Files:
clang/test/CodeGen/ppc64-complex-parms.c
llvm/include/llvm/ADT/APFloat.h
llvm/lib/Support/APFloat.cpp
timshen updated this revision to Diff 81980.
timshen added a comment.
Remove 'else' after return.
Remove 'return' on void type.
https://reviews.llvm.org/D27872
Files:
clang/test/CodeGen/ppc64-complex-parms.c
llvm/include/llvm/ADT/APFloat.h
llvm/lib/Support/APFloat.cpp
jtony added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:800
- void makeLargest(bool Neg) { getIEEE().makeLargest(Neg); }
+ void makeLargest(bool Neg) {
+if (usesLayout(getSemantics())) {
I know it is allowed to return a void function
ahatanak added inline comments.
Comment at: llvm/include/llvm/ADT/APFloat.h:773
+ return U.IEEE.makeZero(Neg);
+} else if (usesLayout(getSemantics())) {
+ return U.Double.makeZero(Neg);
You don't need else after return.
timshen created this revision.
timshen added reviewers: hfinkel, kbarton, iteratee, echristo.
timshen added subscribers: llvm-commits, cfe-commits.
Herald added subscribers: nemanjai, mehdi_amini.
This patch changes the layout of DoubleAPFloat, and adjust all
operations to do either:
1.
26 matches
Mail list logo
