Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On 08/06/14 13:45, DJ Delorie wrote: That's one of the things that would be largely made irrelevant by DJ's proposed changes. Instead of using PSImode, What’s PSImode? PSImode is a mode with more precision than HImode, but less than SImode. we'd be able to define modes of precisely the number of bits one of these targets needs. Not quite, Jeff. We still use PSImode but now it has a known defined precision, not just fits in SImode, and may correspond to one of the __intN types. Ah, cool, I didn't know all that was in. Glad to know that we can precisely define the precision of the partial modes. There's still lots of places in gcc that use SIZE where they should use PRECISION. Yea, I suspect this is rather pervasive. Jeff
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
Ah, cool, I didn't know all that was in. Well, the precision part is in, but the __intN part isn't yet. Each time I do a final check for regressions, something new has been added to gcc which breaks it all again... :-P
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, 6 Aug 2014, Mike Stump wrote:
On Aug 6, 2014, at 12:56 AM, Richard Biener rguent...@suse.de wrote:
My concern is the code we're removing discusses the need to simplify when
these expressions are in static initializers. What's going to ensure that
we're still simplifying instances which appear in static initializers? I
don't see anything which tests that. And does it still work for targets
which utilize PSImode?
You mean stuff like
int i[4];
const int p = i - (i + 2);
No.
long p = i - (i + 2);
in C++. It is valid and so it has to work. Further, there is a well known
problem that if you get all the constants correct, you get to statically
initialize this and have no constructors. Though the above is trivial, in
large C++ source bases, there is an important set of things that one must do
to hit expected quality. Fold, fold, fold is the name of the game, and if
you do it well, you get app startup time that is short, and if you don’t do
it well, you get app startup time that sucks. Some folks care about startup
time. So for example, there are well known pdf viewers that use gcc to
compile and whose startup time would be a order of magnitude or more worse if
gcc didn’t optimize it as well as it does. There are large OS vendors that
had:
It always works - you eventually get a runtime constructor though.
-Wglobal-constructors
Warn about namespace scope data that requires construction or
destruction, or functions that use the constructor
attribute or the destructor attribute. Additionally warn if the
Objective-C GNU runtime is used to initialize
various metadata.
and used it to ensure (-Werror) that no constructors would sneak in anyplace,
as the corporate guide was no, you don’t get to do that. Exceptions are made
and really, the main thing that hurt the most were libraries that users might
use, but you get the idea.
The right way to think of the cost was page faults proportional to the size
of the app, or, 0. Pages faults turn out to be rather expensive as the core
speed of the processor is rather fast. I know sounds overly expensive, but
that is my experience with Ado.., oops, I mean, with one critical customer.
Now, what do you have to do to get it right, evaluate all constant things
including constructors and destructors and be able to do this for a class
hierarchy. We didn’t do if and while, but it would have been nice to do
those. Things like:
class B {
int k;
B(int i) {
k = i;
}
class A :public B {
int i;
int j;
A () :i (1) b(5) {
j = 5;
}
} a;
not that demanding… Note, I tried the code at the top, and gcc generate
runtime code for it. :-( Ouch. clang does this:
_p:
.quad -2 ## 0xfffe
Ah. I’d like to think that used to work at one point.
Probably not ;)
But seriously it's a matter of a) keeping constructors for each
variable separate and optimize them in GIMPLE, b) recognize
resulting code that can be expressed with a initializer
a) can be done with delaying inlining to IPA time and hoping
that early opts do enough to perform the necessary simplification,
b) needs a new pass (but I guess it shouldn't be _that_ hard...)
I think we already have constructor functions marked specially,
hopefully with a link to the initialized variable.
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
err yeah, I guess we'd need to rework things so front end just creates functions for each variable and then middle end pass creates function that call all of them if necessary. There is already code for that in cdtor optimization pass (ipa.c), but I think C++ does its own wrapper to solve orderin issues - but I am not sure abou thtat. Never really cared why those in-frontend wrappers are produced. Honza Should not be hard to add though. Need to read back why inlining would be undesriable here - not inliing ctors in general would indeed be problematic (preventing SRA and more stuff) yeah, doesn't seem that hard. Trev Honza Trev Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Thu, 7 Aug 2014, Jan Hubicka wrote:
Probably not ;)
But seriously it's a matter of a) keeping constructors for each
variable separate and optimize them in GIMPLE, b) recognize
resulting code that can be expressed with a initializer
a) can be done with delaying inlining to IPA time and hoping
that early opts do enough to perform the necessary simplification,
b) needs a new pass (but I guess it shouldn't be _that_ hard...)
I'm not sure totally skipping inlining in C++ is that great an idea
since most C++ ctors will end up calling things. Isn't it enough to not
inline the initializer for a variable into the file level function that
calls all the initializers, which you could do by internally apply
attribute noinline I guess.
I think we already have constructor functions marked specially,
hopefully with a link to the initialized variable.
I'd expect that's what DECL_STATIC_CONSTRUCTOR does, but I'm not sure
there's a link or where we could put one if there isn't.
If you mean language level static constructors, then it doesn't. C++ FE
produce them and then wrap thems in static_construction functions that
are the only having DECL_STATIC_CONSTRUCTOR calling the actual ctors.
Should not be hard to add though. Need to read back why inlining would
be undesriable here - not inliing ctors in general would indeed be problematic
(preventing SRA and more stuff)
It's only important to not inline the static constructor into the
one function calling all of them. And only not doing that during
early inlining (but still inline into those constructors). That's
because we eventually want to eliminate the body of the static
constructor in favor of a DECL_INITIAL in that new pass.
So we need to be able to identify
struct X { int i; X() { i = 1; } } a;
_constructor_of_a ()
{
a.i = 1;
}
and place 'a' into .data with the appropriate initialization.
And then remove a.i = 1
Of course similar thing would be possible from the function
with all those constructors inlined but I guess it's much
harder if you may end up seeing calls inside (stuff may get
overwritten...).
OTOH, I wonder if
struct X { int i; X() { i = 1; } } a;
struct X { int i; X() { i = 1; a.i = 2; } } b;
is well-defined (with proper constructor priority attribute
to init a last or without). That is, may we write to 'a'
from the constructor of b before it has been constructed?
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Thu, Aug 07, 2014 at 01:44:02PM +0200, Jan Hubicka wrote:
It's only important to not inline the static constructor into the
one function calling all of them. And only not doing that during
early inlining (but still inline into those constructors). That's
because we eventually want to eliminate the body of the static
constructor in favor of a DECL_INITIAL in that new pass.
I see, that indeed makes sense (and I was thinking of that).
Disabling early inlining of static ctors seems OK, but we may
not even need to do that given that we process topologically - so
we will identify simple ctor before we inline it.
(assuming we do not want to do this at LTO that would also make
sense)
So we need to be able to identify
struct X { int i; X() { i = 1; } } a;
_constructor_of_a ()
{
a.i = 1;
}
and place 'a' into .data with the appropriate initialization.
And then remove a.i = 1
Yeah, I think annotating cgraph nodes with info this is static
ctor of this variable would be easy and we don't neven need to
mess with the rest of C++ FE finalization code (i.e. one unifying
it to static construction functions).
Early inliner can skip inlining these if desirable and the new
pass can easily identify them.
I'm not sure how you find the variable given a cgraph node, but if you
do great.
Being able to turn stores into ctor is something that may be
shareable with Martin's ipa-prop analysis.
that's handy, have a link?
He is basically doing that IMO.
Of course similar thing would be possible from the function
with all those constructors inlined but I guess it's much
harder if you may end up seeing calls inside (stuff may get
overwritten...).
OTOH, I wonder if
struct X { int i; X() { i = 1; } } a;
struct X { int i; X() { i = 1; a.i = 2; } } b;
is well-defined (with proper constructor priority attribute
to init a last or without). That is, may we write to 'a'
from the constructor of b before it has been constructed?
Hehe, not a question for me :)
I feel like the anser should be if you write to a global before its
I feel like it should only be valid to write to a global variable after
its initialized, but I really don't know what C++ says.
Trev
Honza
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Tue, 5 Aug 2014, Jeff Law wrote:
On 08/05/14 08:36, Marek Polacek wrote:
On Mon, Aug 04, 2014 at 02:04:36PM +0200, Richard Biener wrote:
Looks like .fre can optimize q - (q - 1) into 1:
bb 2:
q.0_3 = (long int) MEM[(void *)i + 4B];
_5 = (long int) i;
- _6 = q.0_3 - _5;
- t.1_7 = _6 /[ex] 4;
- t ={v} t.1_7;
+ t ={v} 1;
i ={v} {CLOBBER};
return;
But associate_plusminus doesn't optimize it:
else if (code == MINUS_EXPR
CONVERT_EXPR_CODE_P (def_code)
TREE_CODE (gimple_assign_rhs1 (def_stmt)) ==
SSA_NAME
TREE_CODE (rhs2) == SSA_NAME)
{
/* (T)(P + A) - (T)P - (T)A. */
becase gimple_assign_rhs1 (def_stmt) is not an SSA_NAME, but ADDR_EXPR
(it's
MEM[(void *)i + 4B]). Then there's transformation A - (A +- B) - -+
B
below, but that doesn't handle casts.
So - should I try to handle it in associate_plusminus?
Yes please, with a (few) testcase(s).
Ok, so the following adds the (T)P - (T)(P + A) - (T)-A
transformation. It is based on code hunk that does the
(T)(P + A) - (T)P - (T)A transformation. The difference it makes is
in the .optimized dump something like:
int fn2(int, int) (int p, int i)
{
- unsigned int p.2_2;
+ unsigned int _3;
int _4;
- unsigned int _5;
unsigned int _6;
- int _7;
bb 2:
- p.2_2 = (unsigned int) p_1(D);
- _4 = p_1(D) + i_3(D);
- _5 = (unsigned int) _4;
- _6 = p.2_2 - _5;
- _7 = (int) _6;
- return _7;
+ _6 = (unsigned int) i_2(D);
+ _3 = -_6;
+ _4 = (int) _3;
+ return _4;
i.e., the PLUS_EXPR and MINUS_EXPR are gone, and NEGATE_EXPR is used
instead.
During bootstrap with --enable-languages=c,c++ this optimization triggered
238 times.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-05 Marek Polacek pola...@redhat.com
PR c/61240
* tree-ssa-forwprop.c (associate_plusminus): Add (T)P - (T)(P + A)
- (T)-A transformation.
c/
* c-typeck.c (pointer_diff): Remove P - (P + CST) optimization.
testsuite/
* gcc.dg/pr61240.c: New test.
* gcc.dg/tree-ssa/forwprop-29.c: New test.
So I'm all for delaying folding when possible, so I'm comfortable with the
general direction this is going.
My concern is the code we're removing discusses the need to simplify when
these expressions are in static initializers. What's going to ensure that
we're still simplifying instances which appear in static initializers? I
don't see anything which tests that. And does it still work for targets
which utilize PSImode?
You mean stuff like
int i[4];
const int p = i - (i + 2);
? I don't think that i - (i + 2) fulfills the requirements of an
integral constant expression, so we are free to reject it.
OTOH, ISTR that we tried hard in the past to support initializers
that implement offsetof in various ways.
Btw, before the idea of moving foldings to GIMPLE the consensus was
that moving foldings from frontends and convert to fold-const.c was
the appropriate thing to do.
As this is offsetof-like moving it to fold-const.c would work for
me as well (also given that forwprop is hardly a generic folding
framework).
Bah, I should accelerate that match-and-simplify stuff ...
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Tue, Aug 05, 2014 at 02:14:21PM -0600, Jeff Law wrote: My concern is the code we're removing discusses the need to simplify when these expressions are in static initializers. What's going to ensure that we're still simplifying instances which appear in static initializers? I don't see anything which tests that. And does it still work for targets which utilize PSImode? Aw nuts. So with the patch we'd start erroring out on static __PTRDIFF_TYPE__ d1 = p - (p + 1); static __PTRDIFF_TYPE__ d2 = p - (p - 1); (it's nowhere in the testsuite/code base - and I hadn't noticed that until today :() while we'd still accept static __PTRDIFF_TYPE__ d5 = (p - 1) - p; static __PTRDIFF_TYPE__ d6 = (p + 1) - p; (Those are not constant expression according to ISO C.) The reason is that fold_build can fold (long int) (p + 4) - (long int) p to 4, but not (long int) p - (long int) (p + 4). That means we have to have a way how to fold the latter, but only in static initializers. So I guess I need to implement this in fold-const.c... Oh well. Nevertheless, I'd guess the fwprop bits could go in separately (it's beneficial for C++). As for PSImode, I dunno - seems only m32c and AVR use that? I have no way how to perform testing on such targets. Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, Aug 06, 2014 at 10:22:19AM +0200, Marek Polacek wrote: On Tue, Aug 05, 2014 at 02:14:21PM -0600, Jeff Law wrote: My concern is the code we're removing discusses the need to simplify when these expressions are in static initializers. What's going to ensure that we're still simplifying instances which appear in static initializers? I don't see anything which tests that. And does it still work for targets which utilize PSImode? Aw nuts. So with the patch we'd start erroring out on static __PTRDIFF_TYPE__ d1 = p - (p + 1); static __PTRDIFF_TYPE__ d2 = p - (p - 1); (it's nowhere in the testsuite/code base - and I hadn't noticed that until today :() while we'd still accept static __PTRDIFF_TYPE__ d5 = (p - 1) - p; static __PTRDIFF_TYPE__ d6 = (p + 1) - p; (Those are not constant expression according to ISO C.) The reason is that fold_build can fold (long int) (p + 4) - (long int) p to 4, but not (long int) p - (long int) (p + 4). That means we have to have a way how to fold the latter, but only in static initializers. So I guess I need to implement this in fold-const.c... Oh well. Nevertheless, I'd guess the fwprop bits could go in separately (it's beneficial for C++). Well, if you are going to implement it in fwprop AND fold-const, then the natural place to fix that would be in *.pd on the match-and-simplify branch. Jakub
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, Aug 06, 2014 at 10:26:29AM +0200, Jakub Jelinek wrote: Well, if you are going to implement it in fwprop AND fold-const, then the natural place to fix that would be in *.pd on the match-and-simplify branch. True. So I guess I'll have to put this one on hold for a while... Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, 6 Aug 2014, Marek Polacek wrote: On Tue, Aug 05, 2014 at 02:14:21PM -0600, Jeff Law wrote: My concern is the code we're removing discusses the need to simplify when these expressions are in static initializers. What's going to ensure that we're still simplifying instances which appear in static initializers? I don't see anything which tests that. And does it still work for targets which utilize PSImode? Aw nuts. So with the patch we'd start erroring out on static __PTRDIFF_TYPE__ d1 = p - (p + 1); static __PTRDIFF_TYPE__ d2 = p - (p - 1); (it's nowhere in the testsuite/code base - and I hadn't noticed that until today :() while we'd still accept static __PTRDIFF_TYPE__ d5 = (p - 1) - p; static __PTRDIFF_TYPE__ d6 = (p + 1) - p; (Those are not constant expression according to ISO C.) The reason is that fold_build can fold (long int) (p + 4) - (long int) p to 4, but not (long int) p - (long int) (p + 4). That means we have to have a way how to fold the latter, but only in static initializers. So I guess I need to implement this in fold-const.c... Oh well. Nevertheless, I'd guess the fwprop bits could go in separately (it's beneficial for C++). As for PSImode, I dunno - seems only m32c and AVR use that? I have no way how to perform testing on such targets. The issue for those targets is mainly how they define their 'sizetype' vs. their pointer types. For m32c IIRC sizetype is 16bits while pointer types are 24bit (PSImode). That means that pointer-to-int conversion is usually widening and that pointer-offsetting cannot access large objects fully. See also /* Allow conversions from pointer type to integral type only if there is no sign or zero extension involved. For targets were the precision of ptrofftype doesn't match that of pointers we need to allow arbitrary conversions to ptrofftype. */ if ((POINTER_TYPE_P (lhs_type) INTEGRAL_TYPE_P (rhs1_type)) || (POINTER_TYPE_P (rhs1_type) INTEGRAL_TYPE_P (lhs_type) (TYPE_PRECISION (rhs1_type) = TYPE_PRECISION (lhs_type) || ptrofftype_p (sizetype return false; which we may restrict better with checking whether the pointer uses a partial integer mode. Not sure how PSImode - SImode extends on RTL? Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, 6 Aug 2014, Marek Polacek wrote: On Wed, Aug 06, 2014 at 10:26:29AM +0200, Jakub Jelinek wrote: Well, if you are going to implement it in fwprop AND fold-const, then the natural place to fix that would be in *.pd on the match-and-simplify branch. True. So I guess I'll have to put this one on hold for a while... You can restrict it to fold-const.c for now. I really hope to get match-and-simplify into mergeable state this month. Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Wed, Aug 06, 2014 at 10:28:13AM +0200, Richard Biener wrote: On Wed, 6 Aug 2014, Marek Polacek wrote: On Wed, Aug 06, 2014 at 10:26:29AM +0200, Jakub Jelinek wrote: Well, if you are going to implement it in fwprop AND fold-const, then the natural place to fix that would be in *.pd on the match-and-simplify branch. True. So I guess I'll have to put this one on hold for a while... You can restrict it to fold-const.c for now. I really hope to get match-and-simplify into mergeable state this month. Okay, I'll look into it. Thanks. Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On 08/06/14 02:27, Richard Biener wrote: which we may restrict better with checking whether the pointer uses a partial integer mode. Not sure how PSImode - SImode extends on RTL? Well, at least on the mn102, I defined both a zero and sign extension for PSI - SI. So whichever one the generic parts of the compiler needed, the backend provided. As to what bits are modified, that's target dependent as the precise size of the partial modes is target dependent. That's one of the things that would be largely made irrelevant by DJ's proposed changes. Instead of using PSImode, we'd be able to define modes of precisely the number of bits one of these targets needs. Jeff
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
As for PSImode, I dunno - seems only m32c and AVR use that? I have no way how to perform testing on such targets. and msp430. m32c and msp430 have simulators in gdb.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Aug 6, 2014, at 9:04 AM, Jeff Law l...@redhat.com wrote: which we may restrict better with checking whether the pointer uses a partial integer mode. Not sure how PSImode - SImode extends on RTL? Well, at least on the mn102, I defined both a zero and sign extension for PSI - SI. So whichever one the generic parts of the compiler needed, the backend provided. I have a port that defines exactly the bit width of all partials. The extending of it is machine independent and obvious. Though, there are places in the backend that just call the conversion operation they are interested in and pretend they can’t understand what’s going on. I tend to prefer the backend know and I prefer to explain to it the width of all the partials. Wait, did I submit that? config/msp430/msp430-modes.def: /* 20-bit address */ PARTIAL_INT_MODE (SI, 20, PSI); ah, yes, I think I did. You scared me for a second. As to what bits are modified, that's target dependent as the precise size of the partial modes is target dependent. GET_MODE_PRECISION (mode) is the precision of all modes. That's one of the things that would be largely made irrelevant by DJ's proposed changes. Instead of using PSImode, What’s PSImode? we'd be able to define modes of precisely the number of bits one of these targets needs. Gosh, we already do that. GET_MODE_PRECISION (mode) is exactly the width: PARTIAL_INT_MODE (MODE, PRECISION, NAME); | declares a mode of class PARTIAL_INT with the same size as | MODE (which must be an INT mode) and precision PREC. | Optionally, NAME is the new name of the mode. NAME is the | name of the mode. | ?
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
Responding wrt DJ's proposed changes: As to what bits are modified, that's target dependent as the precise size of the partial modes is target dependent. GET_MODE_PRECISION (mode) is the precision of all modes. True, but not all the compiler uses that info when it should. There are even testsuite cases that get it wrong. That's one of the things that would be largely made irrelevant by DJ's proposed changes. Instead of using PSImode, What’s PSImode? PSImode is a mode with more precision than HImode, but less than SImode. we'd be able to define modes of precisely the number of bits one of these targets needs. Not quite, Jeff. We still use PSImode but now it has a known defined precision, not just fits in SImode, and may correspond to one of the __intN types. Gosh, we already do that. GET_MODE_PRECISION (mode) is exactly the width: There's still lots of places in gcc that use SIZE where they should use PRECISION. I also added a step that looks for target-defined conversions between, for example, HImode and PSImode (rather than go HI-SI-PSI). Plus, as there was no real C type for partial int modes, a lot of gcc made assumptions about type sizes. The addition of arbitrary __intN types means there are more places in the code that are able to properly handle partial integer modes.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Aug 6, 2014, at 12:56 AM, Richard Biener rguent...@suse.de wrote:
My concern is the code we're removing discusses the need to simplify when
these expressions are in static initializers. What's going to ensure that
we're still simplifying instances which appear in static initializers? I
don't see anything which tests that. And does it still work for targets
which utilize PSImode?
You mean stuff like
int i[4];
const int p = i - (i + 2);
No.
long p = i - (i + 2);
in C++. It is valid and so it has to work. Further, there is a well known
problem that if you get all the constants correct, you get to statically
initialize this and have no constructors. Though the above is trivial, in
large C++ source bases, there is an important set of things that one must do to
hit expected quality. Fold, fold, fold is the name of the game, and if you do
it well, you get app startup time that is short, and if you don’t do it well,
you get app startup time that sucks. Some folks care about startup time. So
for example, there are well known pdf viewers that use gcc to compile and whose
startup time would be a order of magnitude or more worse if gcc didn’t optimize
it as well as it does. There are large OS vendors that had:
-Wglobal-constructors
Warn about namespace scope data that requires construction or
destruction, or functions that use the constructor
attribute or the destructor attribute. Additionally warn if the
Objective-C GNU runtime is used to initialize
various metadata.
and used it to ensure (-Werror) that no constructors would sneak in anyplace,
as the corporate guide was no, you don’t get to do that. Exceptions are made
and really, the main thing that hurt the most were libraries that users might
use, but you get the idea.
The right way to think of the cost was page faults proportional to the size of
the app, or, 0. Pages faults turn out to be rather expensive as the core speed
of the processor is rather fast. I know sounds overly expensive, but that is
my experience with Ado.., oops, I mean, with one critical customer.
Now, what do you have to do to get it right, evaluate all constant things
including constructors and destructors and be able to do this for a class
hierarchy. We didn’t do if and while, but it would have been nice to do those.
Things like:
class B {
int k;
B(int i) {
k = i;
}
class A :public B {
int i;
int j;
A () :i (1) b(5) {
j = 5;
}
} a;
not that demanding… Note, I tried the code at the top, and gcc generate
runtime code for it. :-( Ouch. clang does this:
_p:
.quad -2 ## 0xfffe
Ah. I’d like to think that used to work at one point.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
There's still lots of places in gcc that use SIZE where they should use PRECISION. Yes, and in time, they’ll all get cleaned up. Hopefully not much time, if I can get my patches commit-worthy. I'm actually debugging a pointer-signed-math regression now. I also added a step that looks for target-defined conversions between, for example, HImode and PSImode (rather than go HI-SI-PSI). I hope they always fall back to the usual conversions. On my port, I can move them into gprs and do all the usual gpr instructions on them. The register allocator allocates the mode specific registers and moves them around as appropriate. Yeah, it's a check before the usual conversions that just says if the target has a MODE1 to MODE2 converter of the right type, just use it. Without this check, the only mode conversions are (for example) HI-SI and PSI-SI. The check allows for HI-PSI or QI-PTI or whatever the target has opcodes for.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, Aug 04, 2014 at 02:04:36PM +0200, Richard Biener wrote:
Looks like .fre can optimize q - (q - 1) into 1:
bb 2:
q.0_3 = (long int) MEM[(void *)i + 4B];
_5 = (long int) i;
- _6 = q.0_3 - _5;
- t.1_7 = _6 /[ex] 4;
- t ={v} t.1_7;
+ t ={v} 1;
i ={v} {CLOBBER};
return;
But associate_plusminus doesn't optimize it:
else if (code == MINUS_EXPR
CONVERT_EXPR_CODE_P (def_code)
TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME
TREE_CODE (rhs2) == SSA_NAME)
{
/* (T)(P + A) - (T)P - (T)A. */
becase gimple_assign_rhs1 (def_stmt) is not an SSA_NAME, but ADDR_EXPR (it's
MEM[(void *)i + 4B]). Then there's transformation A - (A +- B) - -+ B
below, but that doesn't handle casts.
So - should I try to handle it in associate_plusminus?
Yes please, with a (few) testcase(s).
Ok, so the following adds the (T)P - (T)(P + A) - (T)-A
transformation. It is based on code hunk that does the
(T)(P + A) - (T)P - (T)A transformation. The difference it makes is
in the .optimized dump something like:
int fn2(int, int) (int p, int i)
{
- unsigned int p.2_2;
+ unsigned int _3;
int _4;
- unsigned int _5;
unsigned int _6;
- int _7;
bb 2:
- p.2_2 = (unsigned int) p_1(D);
- _4 = p_1(D) + i_3(D);
- _5 = (unsigned int) _4;
- _6 = p.2_2 - _5;
- _7 = (int) _6;
- return _7;
+ _6 = (unsigned int) i_2(D);
+ _3 = -_6;
+ _4 = (int) _3;
+ return _4;
i.e., the PLUS_EXPR and MINUS_EXPR are gone, and NEGATE_EXPR is used instead.
During bootstrap with --enable-languages=c,c++ this optimization triggered 238
times.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-05 Marek Polacek pola...@redhat.com
PR c/61240
* tree-ssa-forwprop.c (associate_plusminus): Add (T)P - (T)(P + A)
- (T)-A transformation.
c/
* c-typeck.c (pointer_diff): Remove P - (P + CST) optimization.
testsuite/
* gcc.dg/pr61240.c: New test.
* gcc.dg/tree-ssa/forwprop-29.c: New test.
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index fe9440c..5f46944 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3460,7 +3460,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
addr_space_t as0 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op0)));
addr_space_t as1 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op1)));
tree target_type = TREE_TYPE (TREE_TYPE (op0));
- tree con0, con1, lit0, lit1;
tree orig_op1 = op1;
/* If the operands point into different address spaces, we need to
@@ -3490,7 +3489,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
else
inttype = restype;
-
if (TREE_CODE (target_type) == VOID_TYPE)
pedwarn (loc, OPT_Wpointer_arith,
pointer of type %void *% used in subtraction);
@@ -3498,50 +3496,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
pedwarn (loc, OPT_Wpointer_arith,
pointer to a function used in subtraction);
- /* If the conversion to ptrdiff_type does anything like widening or
- converting a partial to an integral mode, we get a convert_expression
- that is in the way to do any simplifications.
- (fold-const.c doesn't know that the extra bits won't be needed.
- split_tree uses STRIP_SIGN_NOPS, which leaves conversions to a
- different mode in place.)
- So first try to find a common term here 'by hand'; we want to cover
- at least the cases that occur in legal static initializers. */
- if (CONVERT_EXPR_P (op0)
- (TYPE_PRECISION (TREE_TYPE (op0))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op0, 0)
-con0 = TREE_OPERAND (op0, 0);
- else
-con0 = op0;
- if (CONVERT_EXPR_P (op1)
- (TYPE_PRECISION (TREE_TYPE (op1))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op1, 0)
-con1 = TREE_OPERAND (op1, 0);
- else
-con1 = op1;
-
- if (TREE_CODE (con0) == POINTER_PLUS_EXPR)
-{
- lit0 = TREE_OPERAND (con0, 1);
- con0 = TREE_OPERAND (con0, 0);
-}
- else
-lit0 = integer_zero_node;
-
- if (TREE_CODE (con1) == POINTER_PLUS_EXPR)
-{
- lit1 = TREE_OPERAND (con1, 1);
- con1 = TREE_OPERAND (con1, 0);
-}
- else
-lit1 = integer_zero_node;
-
- if (operand_equal_p (con0, con1, 0))
-{
- op0 = lit0;
- op1 = lit1;
-}
-
-
/* First do the subtraction as integers;
then drop through to build the divide operator.
Do not do default conversions on the minus operator
diff --git gcc/testsuite/gcc.dg/pr61240.c gcc/testsuite/gcc.dg/pr61240.c
index e69de29..115e415 100644
--- gcc/testsuite/gcc.dg/pr61240.c
+++ gcc/testsuite/gcc.dg/pr61240.c
@@ -0,0 +1,13 @@
+/* PR c/61240 */
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = p - (p - 1);
+}
diff --git gcc/testsuite/gcc.dg/tree-ssa/forwprop-29.c
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On 08/05/14 08:36, Marek Polacek wrote:
On Mon, Aug 04, 2014 at 02:04:36PM +0200, Richard Biener wrote:
Looks like .fre can optimize q - (q - 1) into 1:
bb 2:
q.0_3 = (long int) MEM[(void *)i + 4B];
_5 = (long int) i;
- _6 = q.0_3 - _5;
- t.1_7 = _6 /[ex] 4;
- t ={v} t.1_7;
+ t ={v} 1;
i ={v} {CLOBBER};
return;
But associate_plusminus doesn't optimize it:
else if (code == MINUS_EXPR
CONVERT_EXPR_CODE_P (def_code)
TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME
TREE_CODE (rhs2) == SSA_NAME)
{
/* (T)(P + A) - (T)P - (T)A. */
becase gimple_assign_rhs1 (def_stmt) is not an SSA_NAME, but ADDR_EXPR (it's
MEM[(void *)i + 4B]). Then there's transformation A - (A +- B) - -+ B
below, but that doesn't handle casts.
So - should I try to handle it in associate_plusminus?
Yes please, with a (few) testcase(s).
Ok, so the following adds the (T)P - (T)(P + A) - (T)-A
transformation. It is based on code hunk that does the
(T)(P + A) - (T)P - (T)A transformation. The difference it makes is
in the .optimized dump something like:
int fn2(int, int) (int p, int i)
{
- unsigned int p.2_2;
+ unsigned int _3;
int _4;
- unsigned int _5;
unsigned int _6;
- int _7;
bb 2:
- p.2_2 = (unsigned int) p_1(D);
- _4 = p_1(D) + i_3(D);
- _5 = (unsigned int) _4;
- _6 = p.2_2 - _5;
- _7 = (int) _6;
- return _7;
+ _6 = (unsigned int) i_2(D);
+ _3 = -_6;
+ _4 = (int) _3;
+ return _4;
i.e., the PLUS_EXPR and MINUS_EXPR are gone, and NEGATE_EXPR is used instead.
During bootstrap with --enable-languages=c,c++ this optimization triggered 238
times.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-05 Marek Polacek pola...@redhat.com
PR c/61240
* tree-ssa-forwprop.c (associate_plusminus): Add (T)P - (T)(P + A)
- (T)-A transformation.
c/
* c-typeck.c (pointer_diff): Remove P - (P + CST) optimization.
testsuite/
* gcc.dg/pr61240.c: New test.
* gcc.dg/tree-ssa/forwprop-29.c: New test.
So I'm all for delaying folding when possible, so I'm comfortable with
the general direction this is going.
My concern is the code we're removing discusses the need to simplify
when these expressions are in static initializers. What's going to
ensure that we're still simplifying instances which appear in static
initializers? I don't see anything which tests that. And does it
still work for targets which utilize PSImode?
Jeff
[C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
This PR is about bogus overflow warning that we issue for e.g.
int *q = i + 1;
q - (q - 1);
because pointer_diff receives p - (p + -1U) which gets simplified to
1U - with overflow. We could drop the overflow flag to suppress the
warning, but I think we should just remove the optimization
altogether. First, FE shouldn't perform such transformations at
all. Second, C++ FE has its own pointer_diff function that doesn't
do such optimization. With this patch, the C FE will generate the
same expression as the C++ FE. It's true that we should try to
optimize this, but not in the front end. It ought to be easy to
write a pattern for match-and-simplify that would handle this.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-04 Marek Polacek pola...@redhat.com
PR c/61240
* c-typeck.c (pointer_diff): Remove P - (P + CST) optimization.
* gcc.dg/pr61240.c: New test.
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index fe9440c..5f46944 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3460,7 +3460,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
addr_space_t as0 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op0)));
addr_space_t as1 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op1)));
tree target_type = TREE_TYPE (TREE_TYPE (op0));
- tree con0, con1, lit0, lit1;
tree orig_op1 = op1;
/* If the operands point into different address spaces, we need to
@@ -3490,7 +3489,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
else
inttype = restype;
-
if (TREE_CODE (target_type) == VOID_TYPE)
pedwarn (loc, OPT_Wpointer_arith,
pointer of type %void *% used in subtraction);
@@ -3498,50 +3496,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
pedwarn (loc, OPT_Wpointer_arith,
pointer to a function used in subtraction);
- /* If the conversion to ptrdiff_type does anything like widening or
- converting a partial to an integral mode, we get a convert_expression
- that is in the way to do any simplifications.
- (fold-const.c doesn't know that the extra bits won't be needed.
- split_tree uses STRIP_SIGN_NOPS, which leaves conversions to a
- different mode in place.)
- So first try to find a common term here 'by hand'; we want to cover
- at least the cases that occur in legal static initializers. */
- if (CONVERT_EXPR_P (op0)
- (TYPE_PRECISION (TREE_TYPE (op0))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op0, 0)
-con0 = TREE_OPERAND (op0, 0);
- else
-con0 = op0;
- if (CONVERT_EXPR_P (op1)
- (TYPE_PRECISION (TREE_TYPE (op1))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op1, 0)
-con1 = TREE_OPERAND (op1, 0);
- else
-con1 = op1;
-
- if (TREE_CODE (con0) == POINTER_PLUS_EXPR)
-{
- lit0 = TREE_OPERAND (con0, 1);
- con0 = TREE_OPERAND (con0, 0);
-}
- else
-lit0 = integer_zero_node;
-
- if (TREE_CODE (con1) == POINTER_PLUS_EXPR)
-{
- lit1 = TREE_OPERAND (con1, 1);
- con1 = TREE_OPERAND (con1, 0);
-}
- else
-lit1 = integer_zero_node;
-
- if (operand_equal_p (con0, con1, 0))
-{
- op0 = lit0;
- op1 = lit1;
-}
-
-
/* First do the subtraction as integers;
then drop through to build the divide operator.
Do not do default conversions on the minus operator
diff --git gcc/testsuite/gcc.dg/pr61240.c gcc/testsuite/gcc.dg/pr61240.c
index e69de29..e90c070 100644
--- gcc/testsuite/gcc.dg/pr61240.c
+++ gcc/testsuite/gcc.dg/pr61240.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Marek Polacek wrote:
This PR is about bogus overflow warning that we issue for e.g.
int *q = i + 1;
q - (q - 1);
because pointer_diff receives p - (p + -1U) which gets simplified to
1U - with overflow. We could drop the overflow flag to suppress the
warning, but I think we should just remove the optimization
altogether. First, FE shouldn't perform such transformations at
all. Second, C++ FE has its own pointer_diff function that doesn't
do such optimization. With this patch, the C FE will generate the
same expression as the C++ FE. It's true that we should try to
optimize this, but not in the front end. It ought to be easy to
write a pattern for match-and-simplify that would handle this.
I think that tree-ssa-forwprop.c already simplifies this in
associate_plusminus with (T)(P + A) - (T)P - (T)A. Well,
maybe not - but then the code should be massages to handle it.
Can you double-check and do that?
Otherwise I agree with removing this (and other) premature
optimizations scattered throughout the frontends (and convert.c).
Thanks,
Richard.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-04 Marek Polacek pola...@redhat.com
PR c/61240
* c-typeck.c (pointer_diff): Remove P - (P + CST) optimization.
* gcc.dg/pr61240.c: New test.
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index fe9440c..5f46944 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3460,7 +3460,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
addr_space_t as0 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op0)));
addr_space_t as1 = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (op1)));
tree target_type = TREE_TYPE (TREE_TYPE (op0));
- tree con0, con1, lit0, lit1;
tree orig_op1 = op1;
/* If the operands point into different address spaces, we need to
@@ -3490,7 +3489,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
else
inttype = restype;
-
if (TREE_CODE (target_type) == VOID_TYPE)
pedwarn (loc, OPT_Wpointer_arith,
pointer of type %void *% used in subtraction);
@@ -3498,50 +3496,6 @@ pointer_diff (location_t loc, tree op0, tree op1)
pedwarn (loc, OPT_Wpointer_arith,
pointer to a function used in subtraction);
- /* If the conversion to ptrdiff_type does anything like widening or
- converting a partial to an integral mode, we get a convert_expression
- that is in the way to do any simplifications.
- (fold-const.c doesn't know that the extra bits won't be needed.
- split_tree uses STRIP_SIGN_NOPS, which leaves conversions to a
- different mode in place.)
- So first try to find a common term here 'by hand'; we want to cover
- at least the cases that occur in legal static initializers. */
- if (CONVERT_EXPR_P (op0)
- (TYPE_PRECISION (TREE_TYPE (op0))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op0, 0)
-con0 = TREE_OPERAND (op0, 0);
- else
-con0 = op0;
- if (CONVERT_EXPR_P (op1)
- (TYPE_PRECISION (TREE_TYPE (op1))
- == TYPE_PRECISION (TREE_TYPE (TREE_OPERAND (op1, 0)
-con1 = TREE_OPERAND (op1, 0);
- else
-con1 = op1;
-
- if (TREE_CODE (con0) == POINTER_PLUS_EXPR)
-{
- lit0 = TREE_OPERAND (con0, 1);
- con0 = TREE_OPERAND (con0, 0);
-}
- else
-lit0 = integer_zero_node;
-
- if (TREE_CODE (con1) == POINTER_PLUS_EXPR)
-{
- lit1 = TREE_OPERAND (con1, 1);
- con1 = TREE_OPERAND (con1, 0);
-}
- else
-lit1 = integer_zero_node;
-
- if (operand_equal_p (con0, con1, 0))
-{
- op0 = lit0;
- op1 = lit1;
-}
-
-
/* First do the subtraction as integers;
then drop through to build the divide operator.
Do not do default conversions on the minus operator
diff --git gcc/testsuite/gcc.dg/pr61240.c gcc/testsuite/gcc.dg/pr61240.c
index e69de29..e90c070 100644
--- gcc/testsuite/gcc.dg/pr61240.c
+++ gcc/testsuite/gcc.dg/pr61240.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Marek
--
Richard Biener rguent...@suse.de
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendorffer
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Marek Polacek wrote:
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Why do you want a warning for (q - 1) - q ? It looks like a perfectly
correct way to say -1 to me (ptrdiff_t is a signed type for a reason).
--
Marc Glisse
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Marc Glisse wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Why do you want a warning for (q - 1) - q ? It looks like a perfectly correct
way to say -1 to me (ptrdiff_t is a signed type for a reason).
But computing object - 1 does not result in a valid pointer,
so p - 1 is what we want to warn about? OTOH (q - 1) - q is fine.
Indeed that POINTER_PLUS_EXPR has an unsigned offset is an implementation
detail.
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, Aug 04, 2014 at 12:26:01PM +0200, Richard Biener wrote: I think that tree-ssa-forwprop.c already simplifies this in associate_plusminus with (T)(P + A) - (T)P - (T)A. Well, maybe not - but then the code should be massages to handle it. Can you double-check and do that? Otherwise I agree with removing this (and other) premature optimizations scattered throughout the frontends (and convert.c). IMHO a precondition of such changes is exactly verifying the middle-end can perform those optimizations later on and if not, teaching the middle-end to optimize that. Jakub
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Jakub Jelinek wrote: On Mon, Aug 04, 2014 at 12:26:01PM +0200, Richard Biener wrote: I think that tree-ssa-forwprop.c already simplifies this in associate_plusminus with (T)(P + A) - (T)P - (T)A. Well, maybe not - but then the code should be massages to handle it. Can you double-check and do that? Otherwise I agree with removing this (and other) premature optimizations scattered throughout the frontends (and convert.c). IMHO a precondition of such changes is exactly verifying the middle-end can perform those optimizations later on and if not, teaching the middle-end to optimize that. Yep. Which is why I said double-check and do that, do that in extend what tree-ssa-forwrpop.c does. Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, Aug 04, 2014 at 12:26:01PM +0200, Richard Biener wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
This PR is about bogus overflow warning that we issue for e.g.
int *q = i + 1;
q - (q - 1);
because pointer_diff receives p - (p + -1U) which gets simplified to
1U - with overflow. We could drop the overflow flag to suppress the
warning, but I think we should just remove the optimization
altogether. First, FE shouldn't perform such transformations at
all. Second, C++ FE has its own pointer_diff function that doesn't
do such optimization. With this patch, the C FE will generate the
same expression as the C++ FE. It's true that we should try to
optimize this, but not in the front end. It ought to be easy to
write a pattern for match-and-simplify that would handle this.
I think that tree-ssa-forwprop.c already simplifies this in
associate_plusminus with (T)(P + A) - (T)P - (T)A. Well,
maybe not - but then the code should be massages to handle it.
Can you double-check and do that?
Looks like .fre can optimize q - (q - 1) into 1:
bb 2:
q.0_3 = (long int) MEM[(void *)i + 4B];
_5 = (long int) i;
- _6 = q.0_3 - _5;
- t.1_7 = _6 /[ex] 4;
- t ={v} t.1_7;
+ t ={v} 1;
i ={v} {CLOBBER};
return;
But associate_plusminus doesn't optimize it:
else if (code == MINUS_EXPR
CONVERT_EXPR_CODE_P (def_code)
TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME
TREE_CODE (rhs2) == SSA_NAME)
{
/* (T)(P + A) - (T)P - (T)A. */
becase gimple_assign_rhs1 (def_stmt) is not an SSA_NAME, but ADDR_EXPR (it's
MEM[(void *)i + 4B]). Then there's transformation A - (A +- B) - -+ B
below, but that doesn't handle casts.
So - should I try to handle it in associate_plusminus?
Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, Aug 04, 2014 at 12:51:06PM +0200, Richard Biener wrote:
On Mon, 4 Aug 2014, Marc Glisse wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Why do you want a warning for (q - 1) - q ? It looks like a perfectly
correct
way to say -1 to me (ptrdiff_t is a signed type for a reason).
It's not that I want the warning there. I can probably drop the two
lines from the test.
But computing object - 1 does not result in a valid pointer,
so p - 1 is what we want to warn about? OTOH (q - 1) - q is fine.
Indeed that POINTER_PLUS_EXPR has an unsigned offset is an implementation
detail.
I'm inclined to think that we shouldn't issue the warning at all, note
how it talks about integer overflow, but this is pointer arithmetic.
We can't really decide at that point whether the pointer is valid I'm
afraid.
Marek
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Marek Polacek wrote:
On Mon, Aug 04, 2014 at 12:26:01PM +0200, Richard Biener wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
This PR is about bogus overflow warning that we issue for e.g.
int *q = i + 1;
q - (q - 1);
because pointer_diff receives p - (p + -1U) which gets simplified to
1U - with overflow. We could drop the overflow flag to suppress the
warning, but I think we should just remove the optimization
altogether. First, FE shouldn't perform such transformations at
all. Second, C++ FE has its own pointer_diff function that doesn't
do such optimization. With this patch, the C FE will generate the
same expression as the C++ FE. It's true that we should try to
optimize this, but not in the front end. It ought to be easy to
write a pattern for match-and-simplify that would handle this.
I think that tree-ssa-forwprop.c already simplifies this in
associate_plusminus with (T)(P + A) - (T)P - (T)A. Well,
maybe not - but then the code should be massages to handle it.
Can you double-check and do that?
Looks like .fre can optimize q - (q - 1) into 1:
bb 2:
q.0_3 = (long int) MEM[(void *)i + 4B];
_5 = (long int) i;
- _6 = q.0_3 - _5;
- t.1_7 = _6 /[ex] 4;
- t ={v} t.1_7;
+ t ={v} 1;
i ={v} {CLOBBER};
return;
But associate_plusminus doesn't optimize it:
else if (code == MINUS_EXPR
CONVERT_EXPR_CODE_P (def_code)
TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME
TREE_CODE (rhs2) == SSA_NAME)
{
/* (T)(P + A) - (T)P - (T)A. */
becase gimple_assign_rhs1 (def_stmt) is not an SSA_NAME, but ADDR_EXPR (it's
MEM[(void *)i + 4B]). Then there's transformation A - (A +- B) - -+ B
below, but that doesn't handle casts.
So - should I try to handle it in associate_plusminus?
Yes please, with a (few) testcase(s).
Thanks,
Richard.
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, 4 Aug 2014, Marek Polacek wrote:
On Mon, Aug 04, 2014 at 12:51:06PM +0200, Richard Biener wrote:
On Mon, 4 Aug 2014, Marc Glisse wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Why do you want a warning for (q - 1) - q ? It looks like a perfectly correct
way to say -1 to me (ptrdiff_t is a signed type for a reason).
It's not that I want the warning there. I can probably drop the two
lines from the test.
Well, no, I think we want those two lines, but to test that there is *no*
warning. The bug is only halfway fixed if we still warn for (q-1)-q. But
maybe you are trying to split the fix in 2 patches, in which case I don't
really mind what the intermediate status is.
But computing object - 1 does not result in a valid pointer,
so p - 1 is what we want to warn about? OTOH (q - 1) - q is fine.
Indeed that POINTER_PLUS_EXPR has an unsigned offset is an implementation
detail.
I'm inclined to think that we shouldn't issue the warning at all, note
how it talks about integer overflow, but this is pointer arithmetic.
We can't really decide at that point whether the pointer is valid I'm
afraid.
Richard is talking about a new, unrelated warning, that would warn for
obj-1. That seems doable. Though warning for p-1 when p is defined as
obj sounds much harder in the front-end.
--
Marc Glisse
Re: [C PATCH] Discard P - (P + CST) optimization in pointer_diff (PR c/61240)
On Mon, Aug 04, 2014 at 02:36:12PM +0200, Marc Glisse wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
On Mon, Aug 04, 2014 at 12:51:06PM +0200, Richard Biener wrote:
On Mon, 4 Aug 2014, Marc Glisse wrote:
On Mon, 4 Aug 2014, Marek Polacek wrote:
+/* { dg-do compile } */
+
+void
+foo (void)
+{
+ volatile __PTRDIFF_TYPE__ t;
+ int i;
+ int *p = i;
+ int *q = i + 1;
+ t = q - (q - 1);
+ t = (q - 1) - q; /* { dg-warning integer overflow in expression } */
+ t = p - (p - 1);
+ t = (p - 1) - p ; /* { dg-warning integer overflow in expression } */
+}
Why do you want a warning for (q - 1) - q ? It looks like a perfectly
correct
way to say -1 to me (ptrdiff_t is a signed type for a reason).
It's not that I want the warning there. I can probably drop the two
lines from the test.
Well, no, I think we want those two lines, but to test that there is *no*
warning. The bug is only halfway fixed if we still warn for (q-1)-q. But
maybe you are trying to split the fix in 2 patches, in which case I don't
really mind what the intermediate status is.
Yeah - I'd like to deal with reimplementing the optimization in fwprop
first. The (p - 1) - p warning will need a different fix.
Marek
