Re: fold() can't fold simple expressions?
On Wed, Sep 14, 2016 at 9:49 PM, Jeff Law wrote: > On 09/14/2016 01:29 PM, Richard Biener wrote: > >> >> It's what match-and-simplify does as well. >> >> I question the need to build GENERIC here though. M-a-s happily gets you >> a simplified expression as sequence of GIMPLE statements. (But does not yet >> provide a way to build a simplified GENERIC expression from GIMPLE IL) >> > At a particular site we're going to have an expression where we want to > evaluate the operands (either to a constant or symbolically). > > For each operand in the expression we recursively raise the query "what is > this operand". THe result (potentially another expression) gets substituted > in for the operand and the process continues recursively. Recursion stops > when you get a constant or when you've walked back "too far" or the > expression gets "too complex". > > Once you get something like (x+y)+z you're no longer gimple. But we need to > store that expression in some form. I'm not sure you absolutely need to store it -- it is present in the IL (in unoptimized form) after all. One reason current VRP symbolic ranges are "limited" is to avoid expensive stuff such as folding (because of all the GC memory churn). Yes -- if something simplifies to a constant we'd like to know, m-a-s lets you restrict results to constants (so it avoids building up any larger expressions). You also have a lattice which should contain any intermediate simplifications to constants. You _may_ miss some more advanced simplifications that require simplified non-constant sub-"trees", but folding never was able to catch all opportunities (think of arbitrary associations necessary to cancel everything to zero in a very large expression). For some areas in the compiler (IVO) we have the tree-affine stuff which has its own mini-IL capable of some more "advanced" simplifications (basically to catch associations). > That could be expressed as a vector of operators/operands. It could be > expressed as a generic tree. Whatever. Generic trees as a reasonably > natural way to store the expression as its built, but fold() those arbitrary > expressions doesn't work. m-a-s may be the answer here, I haven't really > thought about it yet. The question is always full generality / complexity vs. working on a production compiler where compile-time and memory use matter. If you can create a testcase, even if artificial, where we blow up worse than N log N this is bad. [current VRP equivalences are such an example] I believe that VRP for symbolic ranges is not terribly important besides "simple" ranges like you can derive from a < b and c != d. Oh, and if you are re-writing VRP range representation please get rid of anti-ranges -- one "simple" extension of current VRP would get rid of them by changing the current rep of a single range/anti-range to an implicit union of two ranges (with that you can represent an anti-range). Make the number of unioned ranges parametric and you end up with sth quite general. And symbolic ranges should be IMHO represented by maintaining a set of active true/false predicates rather than trying to come up with a "range" representation. And for predicates we'd like to have a mini-IL like tree-affine -- this would be generally useful elsewhere in the compiler as well. Richard.
Re: fold() can't fold simple expressions?
On 09/15/2016 02:06 AM, Richard Biener wrote: On Wed, Sep 14, 2016 at 9:49 PM, Jeff Law wrote: On 09/14/2016 01:29 PM, Richard Biener wrote: It's what match-and-simplify does as well. I question the need to build GENERIC here though. M-a-s happily gets you a simplified expression as sequence of GIMPLE statements. (But does not yet provide a way to build a simplified GENERIC expression from GIMPLE IL) At a particular site we're going to have an expression where we want to evaluate the operands (either to a constant or symbolically). For each operand in the expression we recursively raise the query "what is this operand". THe result (potentially another expression) gets substituted in for the operand and the process continues recursively. Recursion stops when you get a constant or when you've walked back "too far" or the expression gets "too complex". Once you get something like (x+y)+z you're no longer gimple. But we need to store that expression in some form. I'm not sure you absolutely need to store it -- it is present in the IL (in unoptimized form) after all. But we don't want to change the IL :-) We want to analyze the IL to determine properties of SSA_NAMEs on various paths through the CFG. The current scheme of mapping range data to an SSA_NAME was an improvement and that range data is starting to be used in various places in the compiler. But that scheme loses extremely valuable data that warnings can and should be using. We typically have ASSERT_EXPRs at branch points in the CFG. The ASSERT_EXPRs carry range information for a path. However we do not consider an ASSERT_EXPR a true copy. ie, after the merge point of the paths uses continue to refer to the original object (the RHS of the ASSERT_EXPRs). Thus after VRP most of the path sensitive data is dropped on the floor. Aldy explored making ASSERT_EXPRs true copies. It's a simple change to make and certainly gets PHIs at the merge point. So it seems like a good start. The downside is there's a significant compile-time cost (via update_ssa) to this scheme. Furthermore, we believe that we only need path specific data for a subset of the SSA_NAMEs used in conditionals. So much of the time is wasted computing information we don't actually need. Worse yet, copy-prop takes the copy created by the ASSERT_EXPR, propagates it to the RHS of the PHI and the PHI ends up as a degenerate and gets removed -- at which point we've lost the path specific range data again. Ugh. If we tell copy-prop not to propagate those copies until we're done with the range data we run the real risk of making code worse and it's just a gross hack, similar how we have to test if an SSA_NAME is used in an abnormal PHI hack. So the scheme we're exploring now is an on-demand generation of path specific range data using back substitution. It's primarily Andrew's ideas, but just happens to mirror parts of the predicate analysis one finds in tree-ssa-uninit.c and my thoughts on how we should be finding/exploiting path specific equivalences for threading and CSE. That could be expressed as a vector of operators/operands. It could be expressed as a generic tree. Whatever. Generic trees as a reasonably natural way to store the expression as its built, but fold() those arbitrary expressions doesn't work. m-a-s may be the answer here, I haven't really thought about it yet. The question is always full generality / complexity vs. working on a production compiler where compile-time and memory use matter. If you can create a testcase, even if artificial, where we blow up worse than N log N this is bad. [current VRP equivalences are such an example] Absolutely. Oh, and if you are re-writing VRP range representation please get rid of anti-ranges -- one "simple" extension of current VRP would get rid of them by changing the current rep of a single range/anti-range to an implicit union of two ranges (with that you can represent an anti-range). Make the number of unioned ranges parametric and you end up with sth quite general. Andrew's scheme (among other things) is designed to give us that flexibility. One of the things we want to explore is how many ranges we really need in practice. It's at least 2 (so we can support anti-ranges), but we don't have a sense of how useful > 2 is in practice. And symbolic ranges should be IMHO represented by maintaining a set of active true/false predicates rather than trying to come up with a "range" representation. And for predicates we'd like to have a mini-IL like tree-affine -- this would be generally useful elsewhere in the compiler as well. That would be possible as well -- essentially it's a representational issue. If you look in tree-ssa-uninit.c you'll see something like this. I'm still looking for an victim to rip tree-ssa-uninit.c apart so that its predicate analysis can be used in other places. jeff
Re: fold() can't fold simple expressions?
On September 15, 2016 6:21:34 PM GMT+02:00, Jeff Law wrote: >On 09/15/2016 02:06 AM, Richard Biener wrote: >> On Wed, Sep 14, 2016 at 9:49 PM, Jeff Law wrote: >>> On 09/14/2016 01:29 PM, Richard Biener wrote: >>> It's what match-and-simplify does as well. I question the need to build GENERIC here though. M-a-s happily gets you a simplified expression as sequence of GIMPLE statements. (But does not yet provide a way to build a simplified GENERIC expression from GIMPLE IL) >>> At a particular site we're going to have an expression where we >>> want to evaluate the operands (either to a constant or >>> symbolically). >>> >>> For each operand in the expression we recursively raise the query >>> "what is this operand". THe result (potentially another >>> expression) gets substituted in for the operand and the process >>> continues recursively. Recursion stops when you get a constant or >>> when you've walked back "too far" or the expression gets "too >>> complex". >>> >>> Once you get something like (x+y)+z you're no longer gimple. But >>> we need to store that expression in some form. >> >> I'm not sure you absolutely need to store it -- it is present in the >> IL (in unoptimized form) after all. >But we don't want to change the IL :-) We want to analyze the IL to >determine properties of SSA_NAMEs on various paths through the CFG. You don't change the IL but only the lattice used to valueize the SSA names when you walk the IL 'looking up' the optimized expression. Richard. >The current scheme of mapping range data to an SSA_NAME was an >improvement and that range data is starting to be used in various >places >in the compiler. But that scheme loses extremely valuable data that >warnings can and should be using. > >We typically have ASSERT_EXPRs at branch points in the CFG. The >ASSERT_EXPRs carry range information for a path. However we do not >consider an ASSERT_EXPR a true copy. ie, after the merge point of the >paths uses continue to refer to the original object (the RHS of the >ASSERT_EXPRs). > >Thus after VRP most of the path sensitive data is dropped on the floor. > >Aldy explored making ASSERT_EXPRs true copies. It's a simple change to > >make and certainly gets PHIs at the merge point. So it seems like a >good start. > >The downside is there's a significant compile-time cost (via >update_ssa) >to this scheme. Furthermore, we believe that we only need path >specific >data for a subset of the SSA_NAMEs used in conditionals. So much of >the >time is wasted computing information we don't actually need. > >Worse yet, copy-prop takes the copy created by the ASSERT_EXPR, >propagates it to the RHS of the PHI and the PHI ends up as a degenerate > >and gets removed -- at which point we've lost the path specific range >data again. Ugh. > >If we tell copy-prop not to propagate those copies until we're done >with >the range data we run the real risk of making code worse and it's just >a >gross hack, similar how we have to test if an SSA_NAME is used in an >abnormal PHI hack. > >So the scheme we're exploring now is an on-demand generation of path >specific range data using back substitution. It's primarily Andrew's >ideas, but just happens to mirror parts of the predicate analysis one >finds in tree-ssa-uninit.c and my thoughts on how we should be >finding/exploiting path specific equivalences for threading and CSE. > > >> >>> That could be expressed as a vector of operators/operands. It >>> could be expressed as a generic tree. Whatever. Generic trees as >>> a reasonably natural way to store the expression as its built, but >>> fold() those arbitrary expressions doesn't work. m-a-s may be the >>> answer here, I haven't really thought about it yet. >> >> The question is always full generality / complexity vs. working on a >> production compiler where compile-time and memory use matter. If you >> can create a testcase, even if artificial, where we blow up worse >> than N log N this is bad. [current VRP equivalences are such an >> example] >Absolutely. > >> Oh, and if you are re-writing VRP range representation please get rid >> of anti-ranges -- one "simple" extension of current VRP would get rid >> of them by changing the current rep of a single range/anti-range to >> an implicit union of two ranges (with that you can represent an >> anti-range). Make the number of unioned ranges parametric and you end >> up with sth quite general. >Andrew's scheme (among other things) is designed to give us that >flexibility. One of the things we want to explore is how many ranges >we >really need in practice. It's at least 2 (so we can support >anti-ranges), but we don't have a sense of how useful > 2 is in >practice. > >> >> And symbolic ranges should be IMHO represented by maintaining a set >> of active true/false predicates rather than trying to come up with a >> "range" representation. And for predicates we'd like to have a >> mini-I
Message missing from gcc-patches
Hi,
this message did not get listed on the gcc-patches archive.
I've got no bounce, and it just vanished, several times.
Any idea what is wrong?
Bernd.
Forwarded Message
Subject: [PATCHv3, resent] Add a warning for suspicious use of
conditional expressions in boolean context
Date: Thu, 15 Sep 2016 11:19:32 +0200
From: Bernd Edlinger
To: GCC Patches
CC: Jason Merrill , Jeff Law , Joseph
Myers
Hi,
I send the latest version of the warning patch again, because I don't
see the previous patch submission on the gcc-patches list. It dropped
out silently, twice :(
Don't know what went wrong, so please excuse me if this e-mail arrives
duplicate.
On 09/14/16 20:11, Jason Merrill wrote:
>>
>> Yes. The reasoning I initially had was that it is completely
>> pointless to have something of the form "if (x ? 1 : 2)" or
>> "if (x ? 0 : 0)" because the result does not even depend on x
>> in this case. But something like "if (x ? 4999 : 0)" looks
>> bogus but does at least not ignore x.
>>
>> If the false-positives are becoming too much of a problem here,
>> then I should of course revert to the previous heuristic again.
>
> I think we could have both, where the weaker form is part of -Wall and
> people can explicitly select the stronger form.
>
Yes, agreed. So here is what I would think will be the first version.
It can later be extended to cover the more pedantic cases which
will not be enabled by -Wall.
I would like to send a follow-up patch for the warning on
signed-integer shift left in boolean context, which I think
should also be good for Wall.
(I already had that feature in patch version 2 but that's meanwhile
outdated).
Bootstrap and reg-testing on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
gcc:
2016-09-14 Bernd Edlinger
PR c++/77434
* doc/invoke.texi: Document -Wcond-in-bool-context.
PR middle-end/77421
* dwarf2out.c (output_loc_operands): Fix an assertion.
c-family:
2016-09-14 Bernd Edlinger
PR c++/77434
* c.opt (Wcond-in-bool-context): New warning.
* c-common.c (c_common_truthvalue_conversion): Warn on integer
constants in boolean context.
cp:
2016-09-14 Bernd Edlinger
PR c++/77434
* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
testsuite:
2016-09-14 Bernd Edlinger
PR c++/77434
* c-c++-common/Wcond-in-bool-context.c: New test.
Index: gcc/builtins.c
===
--- gcc/builtins.c (revision 240135)
+++ gcc/builtins.c (working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree fndecl
tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
- signbit_call, integer_zero_node);
+ signbit_call, integer_zero_node);
isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
- isinf_call, integer_zero_node);
+ isinf_call, integer_zero_node);
- tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, signbit_call,
- integer_minus_one_node, integer_one_node);
tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
- isinf_call, tmp,
- integer_zero_node);
+ signbit_call, integer_minus_one_node,
+ integer_one_node);
+ /* Avoid a possible -Wint-in-bool-context warning in C. */
+ tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+ integer_zero_node);
+ tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+ isinf_call, tmp, integer_zero_node);
}
return tmp;
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (revision 240135)
+++ gcc/c-family/c-common.c (working copy)
@@ -4652,6 +4652,19 @@ c_common_truthvalue_conversion (location_t locatio
TREE_OPERAND (expr, 0));
case COND_EXPR:
+ if (warn_int_in_bool_context)
+ {
+ tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
+ tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
+ if (TREE_CODE (val1) == INTEGER_CST
+ && TREE_CODE (val2) == INTEGER_CST
+ && !integer_zerop (val1)
+ && !integer_zerop (val2)
+ && (!integer_onep (val1)
+ || !integer_onep (val2)))
+ warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+ "?: using integer constants in boolean context");
+ }
/* Distribute the conversion into the arms of a COND_EXPR. */
if (c_dialect_cxx ())
{
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt (revision 240135)
+++ gcc/c-family/c.opt (working copy)
@@ -545,6 +545,10 @@ Wint-conversion
C ObjC Var(warn_int_conversion) Init(1) Warning
Warn about incompatible integer to pointer and pointer to integer conversions.
+Wint-in-bool-context
+C ObjC C++ ObjC++ V
Re: Message missing from gcc-patches
On 09/15/2016 11:12 AM, Bernd Edlinger wrote: Hi, this message did not get listed on the gcc-patches archive. I've got no bounce, and it just vanished, several times. Any idea what is wrong? No clue. It's not likely hitting the maximum size limits. And I certainly got the copy addressed directly to me. Jeff
gcc-6-20160915 is now available
Snapshot gcc-6-20160915 is now available on ftp://gcc.gnu.org/pub/gcc/snapshots/6-20160915/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 6 SVN branch with the following options: svn://gcc.gnu.org/svn/gcc/branches/gcc-6-branch revision 240167 You'll find: gcc-6-20160915.tar.bz2 Complete GCC MD5=23b41bfd11226dbb7b5017e6d73f8fe1 SHA1=0fb6a9f938e7a566ee232c121e917e78a45587df Diffs from 6-20160908 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-6 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.
