Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Aug 12, 2012, at 1:15 PM, Diego Novillo wrote: + Second, the GCC conding conventions prefer explicit conversion, Spelling... coding
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Tue, Aug 14, 2012 at 10:04 AM, Richard Guenther rguent...@suse.de wrote: On Mon, 13 Aug 2012, Lawrence Crowl wrote: On 8/13/12, Richard Guenther richard.guent...@gmail.com wrote: Increment/decrement operations did not exist, please do not add them at this point. Note that I have also added +=, -= and *= operations. Having them has three advantages. First, it matches expectations on what numeric types allow. Second, it results in more concise code. Third, it results in potentially faster code. I think we should be able to use those operators. When I run through changing call sites, I really want to change the sites to the final form, not do two passes. Ok. Btw, I noticed we now have /* Conversion functions. */ HOST_WIDE_INT to_signed () const; unsigned HOST_WIDE_INT to_unsigned () const; /* Conversion query functions. */ bool fits_unsigned () const; bool fits_signed () const; bool fits (bool uns) const; the function names make no sense - they should be talking about host-wide-ints, because that is what they are about. Thus, /* Conversion functions. */ HOST_WIDE_INT to_signed_hwi () const; unsigned HOST_WIDE_INT to_unsigned_hwi () const; /* Conversion query functions. */ bool fits_unsigned_hwi () const; bool fits_signed_hwi () const; bool fits_hwi (bool uns) const; Likewise for static double_int from_unsigned (unsigned HOST_WIDE_INT cst); static double_int from_signed (HOST_WIDE_INT cst); I'm going to install a patch, after testing, that adjusts the names accordingly. Richard. Thanks, Richard.
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Wed, Aug 15, 2012 at 12:28:58PM +0200, Richard Guenther wrote: the function names make no sense - they should be talking about host-wide-ints, because that is what they are about. Thus, /* Conversion functions. */ HOST_WIDE_INT to_signed_hwi () const; unsigned HOST_WIDE_INT to_unsigned_hwi () const; /* Conversion query functions. */ bool fits_unsigned_hwi () const; bool fits_signed_hwi () const; bool fits_hwi (bool uns) const; Wouldn't uhwi and shwi be better? Both are already widely used within gcc... Likewise for static double_int from_unsigned (unsigned HOST_WIDE_INT cst); static double_int from_signed (HOST_WIDE_INT cst); I'm going to install a patch, after testing, that adjusts the names accordingly. Jakub
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Wed, Aug 15, 2012 at 12:31 PM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Aug 15, 2012 at 12:28:58PM +0200, Richard Guenther wrote: the function names make no sense - they should be talking about host-wide-ints, because that is what they are about. Thus, /* Conversion functions. */ HOST_WIDE_INT to_signed_hwi () const; unsigned HOST_WIDE_INT to_unsigned_hwi () const; /* Conversion query functions. */ bool fits_unsigned_hwi () const; bool fits_signed_hwi () const; bool fits_hwi (bool uns) const; Wouldn't uhwi and shwi be better? Both are already widely used within gcc... True, I'll change it that way. Richard. Likewise for static double_int from_unsigned (unsigned HOST_WIDE_INT cst); static double_int from_signed (HOST_WIDE_INT cst); I'm going to install a patch, after testing, that adjusts the names accordingly. Jakub
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Wed, 15 Aug 2012, Richard Guenther wrote:
On Wed, Aug 15, 2012 at 12:31 PM, Jakub Jelinek ja...@redhat.com wrote:
On Wed, Aug 15, 2012 at 12:28:58PM +0200, Richard Guenther wrote:
the function names make no sense - they should be talking about
host-wide-ints, because that is what they are about. Thus,
/* Conversion functions. */
HOST_WIDE_INT to_signed_hwi () const;
unsigned HOST_WIDE_INT to_unsigned_hwi () const;
/* Conversion query functions. */
bool fits_unsigned_hwi () const;
bool fits_signed_hwi () const;
bool fits_hwi (bool uns) const;
Wouldn't uhwi and shwi be better? Both are already widely used within
gcc...
True, I'll change it that way.
Like so, testing in progress.
Richard.
2012-08-15 Richard Guenther rguent...@suse.de
* double-int.h (from_unsigned): Rename to ...
(from_uhwi): ... this.
(from_signed): Rename to ...
(from_shwi): ... this.
(to_signed): Rename to ...
(to_shwi): ... this.
(to_unsigned): Rename to ...
(to_uhwi): ... this.
(fits_unsigned): Rename to ...
(fits_uhwi): ... this.
(fits_signed): Rename to ...
(fits_shwi): ... this.
(fits): Rename to ...
(fits_hwi): ... this.
* double-int.c: Likewise.
Index: gcc/double-int.c
===
*** gcc/double-int.c(revision 190407)
--- gcc/double-int.c(working copy)
*** double_int::sext (unsigned prec) const
*** 716,722
/* Returns true if CST fits in signed HOST_WIDE_INT. */
bool
! double_int::fits_signed () const
{
const double_int cst = *this;
if (cst.high == 0)
--- 716,722
/* Returns true if CST fits in signed HOST_WIDE_INT. */
bool
! double_int::fits_shwi () const
{
const double_int cst = *this;
if (cst.high == 0)
*** double_int::fits_signed () const
*** 731,742
unsigned HOST_WIDE_INT if UNS is true. */
bool
! double_int::fits (bool uns) const
{
if (uns)
! return this-fits_unsigned ();
else
! return this-fits_signed ();
}
/* Returns A * B. */
--- 731,742
unsigned HOST_WIDE_INT if UNS is true. */
bool
! double_int::fits_hwi (bool uns) const
{
if (uns)
! return this-fits_uhwi ();
else
! return this-fits_shwi ();
}
/* Returns A * B. */
Index: gcc/double-int.h
===
*** gcc/double-int.h(revision 190407)
--- gcc/double-int.h(working copy)
*** public:
*** 60,67
Second, the GCC conding conventions prefer explicit conversion,
and explicit conversion operators are not available until C++11. */
! static double_int from_unsigned (unsigned HOST_WIDE_INT cst);
! static double_int from_signed (HOST_WIDE_INT cst);
/* No copy assignment operator or destructor to keep the type a POD. */
--- 60,67
Second, the GCC conding conventions prefer explicit conversion,
and explicit conversion operators are not available until C++11. */
! static double_int from_uhwi (unsigned HOST_WIDE_INT cst);
! static double_int from_shwi (HOST_WIDE_INT cst);
/* No copy assignment operator or destructor to keep the type a POD. */
*** public:
*** 83,96
/* Conversion functions. */
! HOST_WIDE_INT to_signed () const;
! unsigned HOST_WIDE_INT to_unsigned () const;
/* Conversion query functions. */
! bool fits_unsigned () const;
! bool fits_signed () const;
! bool fits (bool uns) const;
/* Attribute query functions. */
--- 83,96
/* Conversion functions. */
! HOST_WIDE_INT to_shwi () const;
! unsigned HOST_WIDE_INT to_uhwi () const;
/* Conversion query functions. */
! bool fits_uhwi () const;
! bool fits_shwi () const;
! bool fits_hwi (bool uns) const;
/* Attribute query functions. */
*** public:
*** 186,192
HOST_WIDE_INT are filled with the sign bit. */
inline
! double_int double_int::from_signed (HOST_WIDE_INT cst)
{
double_int r;
r.low = (unsigned HOST_WIDE_INT) cst;
--- 186,192
HOST_WIDE_INT are filled with the sign bit. */
inline
! double_int double_int::from_shwi (HOST_WIDE_INT cst)
{
double_int r;
r.low = (unsigned HOST_WIDE_INT) cst;
*** double_int double_int::from_signed (HOST
*** 198,204
static inline double_int
shwi_to_double_int (HOST_WIDE_INT cst)
{
! return double_int::from_signed (cst);
}
/* Some useful constants. */
--- 198,204
static inline double_int
shwi_to_double_int (HOST_WIDE_INT cst)
{
! return double_int::from_shwi (cst);
}
/* Some useful constants. */
*** shwi_to_double_int (HOST_WIDE_INT cst)
*** 206,222
The problem is that a named constant would not be as optimizable,
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Mon, 13 Aug 2012, Lawrence Crowl wrote: On 8/13/12, Richard Guenther richard.guent...@gmail.com wrote: Increment/decrement operations did not exist, please do not add them at this point. Note that I have also added +=, -= and *= operations. Having them has three advantages. First, it matches expectations on what numeric types allow. Second, it results in more concise code. Third, it results in potentially faster code. I think we should be able to use those operators. When I run through changing call sites, I really want to change the sites to the final form, not do two passes. Ok. Thanks, Richard.
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Sun, Aug 12, 2012 at 11:30 PM, Marc Glisse marc.gli...@inria.fr wrote:
On Sun, 12 Aug 2012, Diego Novillo wrote:
This implements the double_int rewrite.
See http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00711.html for
details.
Diego.
I am taking it as a chance to ask a couple questions about the coding
conventions.
2012-08-12 Lawrence Crowl cr...@google.com
* hash-table.h
(typedef double_int): Change to struct (POD).
(double_int::make): New overloads for int to double-int
conversion.
Isn't that double_int::from_* now?
+typedef struct double_int
{
[...]
} double_int;
Does the coding convention say something about this verbosity?
+ HOST_WIDE_INT to_signed () const;
+ unsigned HOST_WIDE_INT to_unsigned () const;
+
+ /* Conversion query functions. */
+
+ bool fits_unsigned() const;
+ bool fits_signed() const;
Space before the parentheses or not?
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
Increment/decrement operations did not exist, please do not add them
at this point.
Richard.
+ return *this;
+}
--
Marc Glisse
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Sun, Aug 12, 2012 at 11:30:59PM +0200, Marc Glisse wrote:
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
Do you mean that *this + double_int_one; alone also works, just is
confusing? That would mean operator+ has side-effects, right?
Jakub
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Mon, 13 Aug 2012, Jakub Jelinek wrote:
On Sun, Aug 12, 2012 at 11:30:59PM +0200, Marc Glisse wrote:
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
Do you mean that *this + double_int_one; alone also works, just is
confusing? That would mean operator+ has side-effects, right?
It works in that it compiles. It is confusing because the addition is
dead code and thus operator++ is a nop. Sorry for my confusing euphemism,
I should have called it a bug. operator+ has no side-effects AFAICS.
Note that there are many obvious places where this operator can be used:
varasm.c: i = double_int_add (i, double_int_one);
tree-vrp.c: prod2h = double_int_add (prod2h, double_int_one);
tree-ssa-loop-niter.c:bound = double_int_add (bound, double_int_one);
tree-ssa-loop-niter.c: *nit = double_int_add (*nit, double_int_one);
tree-ssa-loop-ivopts.c:max_niter = double_int_add (max_niter,
double_int_one);
gimple-fold.c:index = double_int_add (index, double_int_one);
etc.
As a side note, I don't usually like making operator+ a member function.
It doesn't matter when there is no implicit conversion, but if we ever add
them, it will make addition non-symmetric.
--
Marc Glisse
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Mon, Aug 13, 2012 at 5:32 AM, Marc Glisse marc.gli...@inria.fr wrote:
On Mon, 13 Aug 2012, Jakub Jelinek wrote:
On Sun, Aug 12, 2012 at 11:30:59PM +0200, Marc Glisse wrote:
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
Do you mean that *this + double_int_one; alone also works, just is
confusing? That would mean operator+ has side-effects, right?
It works in that it compiles. It is confusing because the addition is dead
code and thus operator++ is a nop. Sorry for my confusing euphemism, I
should have called it a bug. operator+ has no side-effects AFAICS.
yes, it is just as confusing and a bug as
2.3 + 1;
is in plain C.
Note that there are many obvious places where this operator can be used:
varasm.c: i = double_int_add (i, double_int_one);
tree-vrp.c: prod2h = double_int_add (prod2h, double_int_one);
tree-ssa-loop-niter.c:bound = double_int_add (bound, double_int_one);
tree-ssa-loop-niter.c: *nit = double_int_add (*nit, double_int_one);
tree-ssa-loop-ivopts.c:max_niter = double_int_add (max_niter,
double_int_one);
gimple-fold.c:index = double_int_add (index, double_int_one);
etc.
As a side note, I don't usually like making operator+ a member function. It
doesn't matter when there is no implicit conversion, but if we ever add
them, it will make addition non-symmetric.
As not everybody is familiar with C++ litotes, let me expand on this. I believe
you are not opposing overloading operator+ on double_int. You are objecting
to its implementation being defined as a member function. That is you would be
perfectly fine with operator+ defined as a free function, e.g. not a
member function.
-- Gaby
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On 8/13/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
On Aug 13, 2012 Marc Glisse marc.gli...@inria.fr wrote:
On Mon, 13 Aug 2012, Jakub Jelinek wrote:
On Sun, Aug 12, 2012 at 11:30:59PM +0200, Marc Glisse wrote:
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
Do you mean that *this + double_int_one; alone also works,
just is confusing? That would mean operator+ has side-effects,
right?
It works in that it compiles. It is confusing because the
addition is dead code and thus operator++ is a nop. Sorry for
my confusing euphemism, I should have called it a bug. operator+
has no side-effects AFAICS.
yes, it is just as confusing and a bug as
2.3 + 1;
is in plain C.
Yes, it is a bug. It's a bit disturbing that it wasn't caught
in bootstrap.
Note that there are many obvious places where this operator can be used:
varasm.c: i = double_int_add (i, double_int_one);
tree-vrp.c: prod2h = double_int_add (prod2h, double_int_one);
tree-ssa-loop-niter.c:bound = double_int_add (bound, double_int_one);
tree-ssa-loop-niter.c: *nit = double_int_add (*nit, double_int_one);
tree-ssa-loop-ivopts.c:max_niter = double_int_add (max_niter,
double_int_one);
gimple-fold.c:index = double_int_add (index, double_int_one);
etc.
As a side note, I don't usually like making operator+ a
member function. It doesn't matter when there is no implicit
conversion, but if we ever add them, it will make addition
non-symmetric.
As not everybody is familiar with C++ litotes, let me expand
on this. I believe you are not opposing overloading operator+ on
double_int. You are objecting to its implementation being defined
as a member function. That is you would be perfectly fine with
operator+ defined as a free function, e.g. not a member function.
In the absence of symmetric overloading, the code is slightly
cleaner with operators as member functions. It is easy to change
should we need symmetric overloads because, for the most part,
the change would have no effect on client code.
--
Lawrence Crowl
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On 8/12/12, Marc Glisse marc.gli...@inria.fr wrote:
On Sun, 12 Aug 2012, Diego Novillo wrote:
This implements the double_int rewrite.
See http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00711.html for
details.
I am taking it as a chance to ask a couple questions about the coding
conventions.
2012-08-12 Lawrence Crowl cr...@google.com
* hash-table.h
(typedef double_int): Change to struct (POD).
(double_int::make): New overloads for int to double-int conversion.
Isn't that double_int::from_* now?
Yes.
+typedef struct double_int
{
[...]
} double_int;
Does the coding convention say something about this verbosity?
No. It helps to have it in code that is compiled by both C and C++.
In this case, it will only be compiled by C++ and the verbosity
is unnecessary. I left the verbosity as it was to help keep the
diff synchronized. I certainly don't object to a cleanup pass for
this kind of stuff.
+ HOST_WIDE_INT to_signed () const;
+ unsigned HOST_WIDE_INT to_unsigned () const;
+
+ /* Conversion query functions. */
+
+ bool fits_unsigned() const;
+ bool fits_signed() const;
Space before the parentheses or not?
Space. Sorry, gcc is the only coding convention I've used that
requires the space. My fingers sometimes forget.
--
Lawrence Crowl
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On 8/13/12, Richard Guenther richard.guent...@gmail.com wrote: Increment/decrement operations did not exist, please do not add them at this point. Note that I have also added +=, -= and *= operations. Having them has three advantages. First, it matches expectations on what numeric types allow. Second, it results in more concise code. Third, it results in potentially faster code. I think we should be able to use those operators. When I run through changing call sites, I really want to change the sites to the final form, not do two passes. -- Lawrence Crowl
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Mon, Aug 13, 2012 at 5:41 AM, Richard Guenther richard.guent...@gmail.com wrote: *this += double_int_one; would be less confusing. Increment/decrement operations did not exist, please do not add them at this point. But they are going to be used when the call-sites are converted. There is no point in leaving them out now. Diego.
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On 08/13/2012 01:22 PM, Lawrence Crowl wrote: yes, it is just as confusing and a bug as 2.3 + 1; is in plain C. Yes, it is a bug. It's a bit disturbing that it wasn't caught in bootstrap. You'll recall that I pointed it out last time around as well. r~
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On 8/13/12, Richard Henderson r...@redhat.com wrote: On 08/13/2012 01:22 PM, Lawrence Crowl wrote: yes, it is just as confusing and a bug as 2.3 + 1; is in plain C. Yes, it is a bug. It's a bit disturbing that it wasn't caught in bootstrap. You'll recall that I pointed it out last time around as well. My memory must be losing bits. -- Lawrence Crowl
Re: Merge C++ conversion into trunk (5/6 - double_int rewrite)
On Sun, 12 Aug 2012, Diego Novillo wrote:
This implements the double_int rewrite.
See http://gcc.gnu.org/ml/gcc-patches/2012-08/msg00711.html for
details.
Diego.
I am taking it as a chance to ask a couple questions about the coding
conventions.
2012-08-12 Lawrence Crowl cr...@google.com
* hash-table.h
(typedef double_int): Change to struct (POD).
(double_int::make): New overloads for int to double-int conversion.
Isn't that double_int::from_* now?
+typedef struct double_int
{
[...]
} double_int;
Does the coding convention say something about this verbosity?
+ HOST_WIDE_INT to_signed () const;
+ unsigned HOST_WIDE_INT to_unsigned () const;
+
+ /* Conversion query functions. */
+
+ bool fits_unsigned() const;
+ bool fits_signed() const;
Space before the parentheses or not?
+inline double_int
+double_int::operator ++ ()
+{
+ *this + double_int_one;
*this += double_int_one;
would be less confusing.
+ return *this;
+}
--
Marc Glisse
