* 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).