Re: Change double_int calls to new interface.
Date: Tue, 11 Sep 2012 17:03:39 -0700 From: Ian Lance Taylor i...@google.com On Tue, Sep 11, 2012 at 3:12 PM, Lawrence Crowl cr...@googlers.com wrote: On 9/11/12, Andreas Schwab sch...@linux-m68k.org wrote: Mark Kettenis mark.kette...@xs4all.nl writes: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] This is PR54528. The expression itself looks correct. I have not been able to duplicate the problem on x86. I am now waiting on access to the compile farm for access to a hppa system. Does anyone have more specific information on the condition that generates the error? I haven't tried, but I bet you can get it to happen if you build a 32-bit compiler--that is, build a compiler using -m32 so that the compiler itself runs 32 bit code. I don't think that helps since I don't see the problem on OpenBSD/i386 (i386-unknown-openbsd5.2) whith a strictly 32-bit compiler. As I wrote earlier, I suspect the key factor is HOST_WIDE_INT being 32-bit, which means 32-bit architectures that don't sey need_64bit_hwint in config.gcc. The fact that m68k is affected as well strengthens my suspicion. So I expect arm to show the problem as well. But people probably haven't noticed since they cross-compile. Anyway the diff below seems to fix the issue. Guess the replacement doesn't work! Should be a big enough clue for Lawrence to come up with a proper fix. Index: fold-const.c === --- fold-const.c(revision 191150) +++ fold-const.c(working copy) @@ -982,13 +982,15 @@ break; case MINUS_EXPR: -/* FIXME(crowl) Remove this code if the replacment works. + /* FIXME(crowl) Remove this code if the replacment works. */ +#if 1 neg_double (op2.low, op2.high, res.low, res.high); add_double (op1.low, op1.high, res.low, res.high, res.low, res.high); overflow = OVERFLOW_SUM_SIGN (res.high, op2.high, op1.high); -*/ +#else res = op1.add_with_sign (-op2, false, overflow); +#endif break; case MULT_EXPR:
Re: Change double_int calls to new interface.
On 9/12/12, Mark Kettenis mark.kette...@xs4all.nl wrote: Date: Tue, 11 Sep 2012 17:03:39 -0700 From: Ian Lance Taylor i...@google.com On Tue, Sep 11, 2012 at 3:12 PM, Lawrence Crowl cr...@googlers.com wrote: On 9/11/12, Andreas Schwab sch...@linux-m68k.org wrote: Mark Kettenis mark.kette...@xs4all.nl writes: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] This is PR54528. The expression itself looks correct. I have not been able to duplicate the problem on x86. I am now waiting on access to the compile farm for access to a hppa system. Does anyone have more specific information on the condition that generates the error? I haven't tried, but I bet you can get it to happen if you build a 32-bit compiler--that is, build a compiler using -m32 so that the compiler itself runs 32 bit code. I don't think that helps since I don't see the problem on OpenBSD/i386 (i386-unknown-openbsd5.2) whith a strictly 32-bit compiler. As I wrote earlier, I suspect the key factor is HOST_WIDE_INT being 32-bit, which means 32-bit architectures that don't sey need_64bit_hwint in config.gcc. The fact that m68k is affected as well strengthens my suspicion. So I expect arm to show the problem as well. But people probably haven't noticed since they cross-compile. Anyway the diff below seems to fix the issue. Guess the replacement doesn't work! Should be a big enough clue for Lawrence to come up with a proper fix. Index: fold-const.c === --- fold-const.c (revision 191150) +++ fold-const.c (working copy) @@ -982,13 +982,15 @@ break; case MINUS_EXPR: -/* FIXME(crowl) Remove this code if the replacment works. + /* FIXME(crowl) Remove this code if the replacment works. */ +#if 1 neg_double (op2.low, op2.high, res.low, res.high); add_double (op1.low, op1.high, res.low, res.high, res.low, res.high); overflow = OVERFLOW_SUM_SIGN (res.high, op2.high, op1.high); -*/ +#else res = op1.add_with_sign (-op2, false, overflow); +#endif break; case MULT_EXPR: My suspicions were on the shift operation. I will update the last patch I sent out with a fix. I will probably need help testing. -- Lawrence Crowl
Re: Change double_int calls to new interface.
Index: gcc/ChangeLog 2012-09-04 Lawrence Crowl cr...@google.com * double-int.h (double_int::operator =): New. (double_int::operator ^=): New. (double_int::operator |=): New. (double_int::mul_with_sign): Modify overflow parameter to bool*. (double_int::add_with_sign): New. (double_int::ule): New. (double_int::sle): New. (binary double_int::operator *): Remove parameter name. (binary double_int::operator +): Likewise. (binary double_int::operator -): Likewise. (binary double_int::operator ): Likewise. (double_int::operator |): Likewise. (double_int::operator ^): Likewise. (double_int::and_not): Likewise. (double_int::from_shwi): Tidy formatting. (double_int::from_uhwi): Likewise. (double_int::from_uhwi): Likewise. * double-int.c (double_int::mul_with_sign): Modify overflow parameter to bool*. (double_int::add_with_sign): New. (double_int::ule): New. (double_int::sle): New. * builtins.c: Modify to use the new double_int interface. * cgraph.c: Likewise. * combine.c: Likewise. * dwarf2out.c: Likewise. * emit-rtl.c: Likewise. * expmed.c: Likewise. * expr.c: Likewise. * fixed-value.c: Likewise. * fold-const.c: Likewise. * gimple-fold.c: Likewise. * gimple-ssa-strength-reduction.c: Likewise. * gimplify-rtx.c: Likewise. * ipa-prop.c: Likewise. * loop-iv.c: Likewise. * optabs.c: Likewise. * stor-layout.c: Likewise. * tree-affine.c: Likewise. * tree-cfg.c: Likewise. * tree-dfa.c: Likewise. * tree-flow-inline.h: Likewise. * tree-object-size.c: Likewise. * tree-predcom.c: Likewise. * tree-pretty-print.c: Likewise. * tree-sra.c: Likewise. * tree-ssa-address.c: Likewise. * tree-ssa-alias.c: Likewise. * tree-ssa-ccp.c: Likewise. * tree-ssa-forwprop.c: Likewise. * tree-ssa-loop-ivopts.c: Likewise. * tree-ssa-loop-niter.c: Likewise. * tree-ssa-phiopt.c: Likewise. * tree-ssa-pre.c: Likewise. * tree-ssa-sccvn: Likewise. * tree-ssa-structalias.c: Likewise. * tree-ssa.c: Likewise. * tree-switch-conversion.c: Likewise. * tree-vect-loop-manip.c: Likewise. * tree-vrp.c: Likewise. * tree.h: Likewise. * tree.c: Likewise. * varasm.c: Likewise. I fear this has broken hppa. Bootstrap on OpenBSD/hppa now fails with: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] ? ~ (t) 0 (sizeof(t) * CHAR_BIT - 1) : (t) 0)) ^ ../../../src/gcc/gcc/system.h:289:44: note: in expansion of macro 'INTTYPE_MINIMUM' #define INTTYPE_MAXIMUM(t) ((t) (~ (t) 0 - INTTYPE_MINIMUM (t))) ^ ../../../src/gcc/gcc/mcf.c:55:22: note: in expansion of macro 'INTTYPE_MAXIMUM' #define CAP_INFINITY INTTYPE_MAXIMUM (HOST_WIDEST_INT) ^ ../../../src/gcc/gcc/mcf.c:211:34: note: in expansion of macro 'CAP_INFINITY' if (fedge-max_capacity == CAP_INFINITY) ^ Something must be wrong with the overflow detection logic in the new double_int interfaces. I suspect this is because for hppa HOST_WIDE_INT is 32 bits wide, since on i386 and x86_64 I don't hit this.
Re: Change double_int calls to new interface.
Mark Kettenis mark.kette...@xs4all.nl writes: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] This is PR54528. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: Change double_int calls to new interface.
On 9/11/12, Andreas Schwab sch...@linux-m68k.org wrote: Mark Kettenis mark.kette...@xs4all.nl writes: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] This is PR54528. The expression itself looks correct. I have not been able to duplicate the problem on x86. I am now waiting on access to the compile farm for access to a hppa system. Does anyone have more specific information on the condition that generates the error? -- Lawrence Crowl
Re: Change double_int calls to new interface.
On Tue, Sep 11, 2012 at 3:12 PM, Lawrence Crowl cr...@googlers.com wrote: On 9/11/12, Andreas Schwab sch...@linux-m68k.org wrote: Mark Kettenis mark.kette...@xs4all.nl writes: In file included from ../../../src/gcc/gcc/mcf.c:47:0: ../../../src/gcc/gcc/mcf.c: In function 'void dump_fixup_edge(FILE*, fixup_graph_type*, fixup_edge_p)': ../../../src/gcc/gcc/system.h:288:78: error: integer overflow in expression [-Werror=overflow] This is PR54528. The expression itself looks correct. I have not been able to duplicate the problem on x86. I am now waiting on access to the compile farm for access to a hppa system. Does anyone have more specific information on the condition that generates the error? I haven't tried, but I bet you can get it to happen if you build a 32-bit compiler--that is, build a compiler using -m32 so that the compiler itself runs 32 bit code. Ian
Re: Change double_int calls to new interface.
On Wed, 5 Sep 2012, Lawrence Crowl wrote:
On 9/5/12, Richard Guenther rguent...@suse.de wrote:
On Tue, 4 Sep 2012, Lawrence Crowl wrote:
Modify gcc/*.[hc] double_int call sites to use the new interface.
This change entailed adding a few new methods to double_int.
Other changes will happen in separate patches. Once all uses of
the old interface are gone, they will be removed.
The change results in a 0.163% time improvement with a 70%
confidence.
Tested on x86_64.
Index: gcc/ChangeLog
- double_int_lshift
- (double_int_one,
-TREE_INT_CST_LOW (vr1.min),
-TYPE_PRECISION (expr_type),
-false));
+ double_int_one
+ .llshift (TREE_INT_CST_LOW
(vr1.min),
+ TYPE_PRECISION
(expr_type)));
Ick - is that what our coding-conventions say? I mean the
.llshift on the next line.
Our conventions say nothing about that, but method calls seem
somewhat analogoust to binary operators, and hence this formatting
was probably the least objectional.
Otherwise ok.
As in you want me to do something else?
No, just asking ;)
The tmin.cmp (tmax, uns) 0 kind of things look odd - definitely
methods like tmin.gt (tmax, uns) would be nice to have. Or even
better, get rid of the 'uns' parameters and provide a
struct double_int_with_signedness {
double_int val;
bool uns;
};
struct double_uint : double_int_with_signedness {
double_uint (double_int);
};
...
and comparison operators which take double_uint/sint.
It would, I think, be better to have separate signed and unsigned
types. That change was significantly structural, and I don't know
where the wide_int work sits in relation to that choice.
double_int is supposed to be a twos-complement thing, ontop of it
we have functions that model signed/unsigned ints of a specific
precision.
You didn't remove any of the old interfaces, so I think we are
going to bitrot quickly again.
I couldn't remove the old interface yet because I haven't updated
all the code yet.
Ah, I see.
Richard.
Re: Change double_int calls to new interface.
On 9/5/12, Richard Guenther rguent...@suse.de wrote:
On Tue, 4 Sep 2012, Lawrence Crowl wrote:
Modify gcc/*.[hc] double_int call sites to use the new interface.
This change entailed adding a few new methods to double_int.
Other changes will happen in separate patches. Once all uses of
the old interface are gone, they will be removed.
The change results in a 0.163% time improvement with a 70%
confidence.
Tested on x86_64.
Index: gcc/ChangeLog
- double_int_lshift
- (double_int_one,
-TREE_INT_CST_LOW (vr1.min),
-TYPE_PRECISION (expr_type),
-false));
+ double_int_one
+ .llshift (TREE_INT_CST_LOW
(vr1.min),
+ TYPE_PRECISION
(expr_type)));
Ick - is that what our coding-conventions say? I mean the
.llshift on the next line.
Our conventions say nothing about that, but method calls seem
somewhat analogoust to binary operators, and hence this formatting
was probably the least objectional.
Otherwise ok.
As in you want me to do something else?
The tmin.cmp (tmax, uns) 0 kind of things look odd - definitely
methods like tmin.gt (tmax, uns) would be nice to have. Or even
better, get rid of the 'uns' parameters and provide a
struct double_int_with_signedness {
double_int val;
bool uns;
};
struct double_uint : double_int_with_signedness {
double_uint (double_int);
};
...
and comparison operators which take double_uint/sint.
It would, I think, be better to have separate signed and unsigned
types. That change was significantly structural, and I don't know
where the wide_int work sits in relation to that choice.
You didn't remove any of the old interfaces, so I think we are
going to bitrot quickly again.
I couldn't remove the old interface yet because I haven't updated
all the code yet.
--
Lawrence Crowl
Re: Change double_int calls to new interface.
On Wed, 5 Sep 2012, Lawrence Crowl wrote:
On 9/5/12, Richard Guenther rguent...@suse.de wrote:
The tmin.cmp (tmax, uns) 0 kind of things look odd - definitely
methods like tmin.gt (tmax, uns) would be nice to have. Or even
better, get rid of the 'uns' parameters and provide a
struct double_int_with_signedness {
double_int val;
bool uns;
};
struct double_uint : double_int_with_signedness {
double_uint (double_int);
};
...
and comparison operators which take double_uint/sint.
It would, I think, be better to have separate signed and unsigned
types. That change was significantly structural, and I don't know
where the wide_int work sits in relation to that choice.
Note that in tree-vrp.c, if I remember correctly, I used both signed and
unsigned operations on the same object (emulating arbitrary precision is
a pain).
--
Marc Glisse
Re: Change double_int calls to new interface.
On 9/5/12, Marc Glisse marc.gli...@inria.fr wrote:
On Wed, 5 Sep 2012, Lawrence Crowl wrote:
On 9/5/12, Richard Guenther rguent...@suse.de wrote:
The tmin.cmp (tmax, uns) 0 kind of things look odd - definitely
methods like tmin.gt (tmax, uns) would be nice to have. Or even
better, get rid of the 'uns' parameters and provide a
struct double_int_with_signedness {
double_int val;
bool uns;
};
struct double_uint : double_int_with_signedness {
double_uint (double_int);
};
...
and comparison operators which take double_uint/sint.
It would, I think, be better to have separate signed and unsigned
types. That change was significantly structural, and I don't know
where the wide_int work sits in relation to that choice.
Note that in tree-vrp.c, if I remember correctly, I used both signed and
unsigned operations on the same object (emulating arbitrary precision is
a pain).
Presumably the wide_int work will address that issue.
--
Lawrence Crowl
