* RFA: PATCH to match.pd for c++/68385
@ 2015-11-20 19:58 Jason Merrill
2015-11-20 20:38 ` Jason Merrill
2015-11-21 6:31 ` Richard Biener
0 siblings, 2 replies; 7+ messages in thread
From: Jason Merrill @ 2015-11-20 19:58 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches List, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 673 bytes --]
In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
Because of delayed folding, the operands aren't fully folded yet, so we
have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
ICEs. We've been seeing several similar bugs, where code calls
integer_zerop and therefore assumes that they have an INTEGER_CST, but
in fact integer_zerop does STRIP_NOPS.
This patch changes the pattern to only match if the operand is actually
an INTEGER_CST. Alternatively we could call tree_strip_nop_conversions
on the operand, but I would expect that to have issues when the
conversion changes the signedness of the type.
OK if testing passes?
[-- Attachment #2: 68385.patch --]
[-- Type: text/x-patch, Size: 778 bytes --]
commit e7b45ed6775c88c6d48c5863738ba0db2e38fc5e
Author: Jason Merrill <jason@redhat.com>
Date: Fri Nov 20 14:40:35 2015 -0500
PR c++/68385
* match.pd: Don't assume that integer_pow2p implies INTEGER_CST.
diff --git a/gcc/match.pd b/gcc/match.pd
index e86cc8b..1981ae7 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -2232,6 +2232,7 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& (TYPE_PRECISION (TREE_TYPE (@0))
== GET_MODE_PRECISION (TYPE_MODE (TREE_TYPE (@0))))
&& element_precision (@2) >= element_precision (@0)
+ && TREE_CODE (@1) == INTEGER_CST
&& wi::only_sign_bit_p (@1, element_precision (@0)))
(with { tree stype = signed_type_for (TREE_TYPE (@0)); }
(ncmp (convert:stype @0) { build_zero_cst (stype); })))))
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-20 19:58 RFA: PATCH to match.pd for c++/68385 Jason Merrill
@ 2015-11-20 20:38 ` Jason Merrill
2015-11-21 6:31 ` Richard Biener
1 sibling, 0 replies; 7+ messages in thread
From: Jason Merrill @ 2015-11-20 20:38 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches List, Ramana Radhakrishnan
On 11/20/2015 02:58 PM, Jason Merrill wrote:
> OK if testing passes?
Which it did.
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-20 19:58 RFA: PATCH to match.pd for c++/68385 Jason Merrill
2015-11-20 20:38 ` Jason Merrill
@ 2015-11-21 6:31 ` Richard Biener
2015-11-21 19:19 ` Marc Glisse
2015-11-23 19:07 ` Jason Merrill
1 sibling, 2 replies; 7+ messages in thread
From: Richard Biener @ 2015-11-21 6:31 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List, Ramana Radhakrishnan
On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill <jason@redhat.com> wrote:
>In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
>Because of delayed folding, the operands aren't fully folded yet, so we
>
>have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
>ICEs. We've been seeing several similar bugs, where code calls
>integer_zerop and therefore assumes that they have an INTEGER_CST, but
>in fact integer_zerop does STRIP_NOPS.
>
>This patch changes the pattern to only match if the operand is actually
>
>an INTEGER_CST. Alternatively we could call tree_strip_nop_conversions
>
>on the operand, but I would expect that to have issues when the
>conversion changes the signedness of the type.
>
>OK if testing passes?
What happens if we remove the nops stripping from integer_zerop? Do other integer predicates strip nops?
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-21 6:31 ` Richard Biener
@ 2015-11-21 19:19 ` Marc Glisse
2015-11-23 10:17 ` Richard Biener
2015-11-23 19:07 ` Jason Merrill
1 sibling, 1 reply; 7+ messages in thread
From: Marc Glisse @ 2015-11-21 19:19 UTC (permalink / raw)
To: Richard Biener; +Cc: Jason Merrill, gcc-patches List, Ramana Radhakrishnan
On Sat, 21 Nov 2015, Richard Biener wrote:
> On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill <jason@redhat.com> wrote:
>> In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
>> Because of delayed folding, the operands aren't fully folded yet, so we
>>
>> have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
>> ICEs. We've been seeing several similar bugs, where code calls
>> integer_zerop and therefore assumes that they have an INTEGER_CST, but
>> in fact integer_zerop does STRIP_NOPS.
>>
>> This patch changes the pattern to only match if the operand is actually
>>
>> an INTEGER_CST. Alternatively we could call tree_strip_nop_conversions
>>
>> on the operand, but I would expect that to have issues when the
>> conversion changes the signedness of the type.
>>
>> OK if testing passes?
>
> What happens if we remove the nops stripping from integer_zerop?
I had the same reaction.
> Do other integer predicates strip nops?
Yes, they do.
I believe I added one or two of those, and the reason I added STRIP_NOPS
is because they started as a copy-paste of integer_zerop...
--
Marc Glisse
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-21 19:19 ` Marc Glisse
@ 2015-11-23 10:17 ` Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2015-11-23 10:17 UTC (permalink / raw)
To: GCC Patches; +Cc: Jason Merrill, Ramana Radhakrishnan
On Sat, Nov 21, 2015 at 7:57 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Sat, 21 Nov 2015, Richard Biener wrote:
>
>> On November 20, 2015 8:58:15 PM GMT+01:00, Jason Merrill
>> <jason@redhat.com> wrote:
>>>
>>> In this bug, we hit the (A & sign-bit) != 0 -> A < 0 transformation.
>>> Because of delayed folding, the operands aren't fully folded yet, so we
>>>
>>> have NOP_EXPRs around INTEGER_CSTs, and so calling wi::only_sign_bit_p
>>> ICEs. We've been seeing several similar bugs, where code calls
>>> integer_zerop and therefore assumes that they have an INTEGER_CST, but
>>> in fact integer_zerop does STRIP_NOPS.
>>>
>>> This patch changes the pattern to only match if the operand is actually
>>>
>>> an INTEGER_CST. Alternatively we could call tree_strip_nop_conversions
>>>
>>> on the operand, but I would expect that to have issues when the
>>> conversion changes the signedness of the type.
>>>
>>> OK if testing passes?
>>
>>
>> What happens if we remove the nops stripping from integer_zerop?
>
>
> I had the same reaction.
>
>> Do other integer predicates strip nops?
>
>
> Yes, they do.
>
> I believe I added one or two of those, and the reason I added STRIP_NOPS is
> because they started as a copy-paste of integer_zerop...
Ok...
Jason, from looking at the PRs backtrace I see the C++ FE does things like
if (complain & tf_warning)
warn_logical_operator (loc, code, boolean_type_node,
code_orig_arg1, fold (arg1),
code_orig_arg2, fold (arg2));
but that's in principle a no-no, if arg1s operands are not folded.
Delayed folding needs
to happen recursively, bottom-up. Folders generally do not expect
unfolded operands
like (int) 1.
There is c-common.c:c_fully_fold () which does this properly but with
/* This function is not relevant to C++ because C++ folds while
parsing, and may need changes to be correct for C++ when C++
stops folding while parsing. */
if (c_dialect_cxx ())
gcc_unreachable ();
not sure if the C++ FE can re-use this for the diagnostic cases.
Richard.
> --
> Marc Glisse
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-21 6:31 ` Richard Biener
2015-11-21 19:19 ` Marc Glisse
@ 2015-11-23 19:07 ` Jason Merrill
2015-11-24 9:40 ` Richard Biener
1 sibling, 1 reply; 7+ messages in thread
From: Jason Merrill @ 2015-11-23 19:07 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches List, Ramana Radhakrishnan
[-- Attachment #1: Type: text/plain, Size: 258 bytes --]
On 11/21/2015 01:30 AM, Richard Biener wrote:
> What happens if we remove the nops stripping from integer_zerop? Do other integer predicates strip nops?
Many predicates do, but removing that doesn't break anything in the
testsuite. So, how about this?
[-- Attachment #2: 68385-2.patch --]
[-- Type: text/x-patch, Size: 3291 bytes --]
commit b4714ac166ce22b54e89ebb860d52637a210c550
Author: Jason Merrill <jason@redhat.com>
Date: Sat Nov 21 07:45:01 2015 -0500
PR c++/68385
* tree.c (integer_zerop, integer_onep, integer_each_onep)
(integer_all_onesp, integer_minus_onep, integer_pow2p)
(integer_nonzerop, integer_truep, tree_log2, tree_floor_log2)
(real_zerop, real_onep, real_minus_onep): Remove STRIP_NOPS.
diff --git a/gcc/tree.c b/gcc/tree.c
index 779fe93..01b2aa8 100644
--- a/gcc/tree.c
+++ b/gcc/tree.c
@@ -2273,8 +2273,6 @@ zerop (const_tree expr)
int
integer_zerop (const_tree expr)
{
- STRIP_NOPS (expr);
-
switch (TREE_CODE (expr))
{
case INTEGER_CST:
@@ -2301,8 +2299,6 @@ integer_zerop (const_tree expr)
int
integer_onep (const_tree expr)
{
- STRIP_NOPS (expr);
-
switch (TREE_CODE (expr))
{
case INTEGER_CST:
@@ -2329,8 +2325,6 @@ integer_onep (const_tree expr)
int
integer_each_onep (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST)
return (integer_onep (TREE_REALPART (expr))
&& integer_onep (TREE_IMAGPART (expr)));
@@ -2344,8 +2338,6 @@ integer_each_onep (const_tree expr)
int
integer_all_onesp (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST
&& integer_all_onesp (TREE_REALPART (expr))
&& integer_all_onesp (TREE_IMAGPART (expr)))
@@ -2371,8 +2363,6 @@ integer_all_onesp (const_tree expr)
int
integer_minus_onep (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST)
return (integer_all_onesp (TREE_REALPART (expr))
&& integer_zerop (TREE_IMAGPART (expr)));
@@ -2386,8 +2376,6 @@ integer_minus_onep (const_tree expr)
int
integer_pow2p (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST
&& integer_pow2p (TREE_REALPART (expr))
&& integer_zerop (TREE_IMAGPART (expr)))
@@ -2405,8 +2393,6 @@ integer_pow2p (const_tree expr)
int
integer_nonzerop (const_tree expr)
{
- STRIP_NOPS (expr);
-
return ((TREE_CODE (expr) == INTEGER_CST
&& !wi::eq_p (expr, 0))
|| (TREE_CODE (expr) == COMPLEX_CST
@@ -2421,8 +2407,6 @@ integer_nonzerop (const_tree expr)
int
integer_truep (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == VECTOR_CST)
return integer_all_onesp (expr);
return integer_onep (expr);
@@ -2443,8 +2427,6 @@ fixed_zerop (const_tree expr)
int
tree_log2 (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST)
return tree_log2 (TREE_REALPART (expr));
@@ -2457,8 +2439,6 @@ tree_log2 (const_tree expr)
int
tree_floor_log2 (const_tree expr)
{
- STRIP_NOPS (expr);
-
if (TREE_CODE (expr) == COMPLEX_CST)
return tree_log2 (TREE_REALPART (expr));
@@ -2582,8 +2562,6 @@ tree_ctz (const_tree expr)
int
real_zerop (const_tree expr)
{
- STRIP_NOPS (expr);
-
switch (TREE_CODE (expr))
{
case REAL_CST:
@@ -2612,8 +2590,6 @@ real_zerop (const_tree expr)
int
real_onep (const_tree expr)
{
- STRIP_NOPS (expr);
-
switch (TREE_CODE (expr))
{
case REAL_CST:
@@ -2641,8 +2617,6 @@ real_onep (const_tree expr)
int
real_minus_onep (const_tree expr)
{
- STRIP_NOPS (expr);
-
switch (TREE_CODE (expr))
{
case REAL_CST:
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: RFA: PATCH to match.pd for c++/68385
2015-11-23 19:07 ` Jason Merrill
@ 2015-11-24 9:40 ` Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2015-11-24 9:40 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches List, Ramana Radhakrishnan
On Mon, Nov 23, 2015 at 8:05 PM, Jason Merrill <jason@redhat.com> wrote:
> On 11/21/2015 01:30 AM, Richard Biener wrote:
>>
>> What happens if we remove the nops stripping from integer_zerop? Do other
>> integer predicates strip nops?
>
>
> Many predicates do, but removing that doesn't break anything in the
> testsuite. So, how about this?
I like it. Ok if you also tested non-standard languages (ada,obj-c++,go).
Thanks,
Richard.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-11-24 9:36 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-20 19:58 RFA: PATCH to match.pd for c++/68385 Jason Merrill
2015-11-20 20:38 ` Jason Merrill
2015-11-21 6:31 ` Richard Biener
2015-11-21 19:19 ` Marc Glisse
2015-11-23 10:17 ` Richard Biener
2015-11-23 19:07 ` Jason Merrill
2015-11-24 9:40 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).