[PATCH] Fix PRs 66502 and 67167
Given there is now PR67167 I am going forward with the earlier posted
patch to switch SCCVN to PHI elimination in favor of another PHI
(to remove IVs) rather than in favor of its only executable edge value.
I still see no way to capture both cases without detecting the choice
and re-numbering the SCC twice, eventually choosing the "better" outcome.
And then the situation where both cases happen in the same SCC is not
handled either.
Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
Richard.
2015-08-13 Richard Biener
PR tree-optimization/66502
PR tree-optimization/67167
* tree-ssa-sccvn.c (vn_phi_compute_hash): Do not include
backedge arguments.
(vn_phi_lookup): Adjust.
(vn_phi_insert): Likewise.
(visit_phi): Prefer to value-number to another PHI node
over value-numbering to a PHI argument.
(init_scc_vn): Mark DFS back edges.
* gcc.dg/tree-ssa/ssa-fre-46.c: New testcase.
Index: gcc/tree-ssa-sccvn.c
===
--- gcc/tree-ssa-sccvn.c(revision 226807)
+++ gcc/tree-ssa-sccvn.c(working copy)
@@ -2655,17 +2655,24 @@ static inline hashval_t
vn_phi_compute_hash (vn_phi_t vp1)
{
inchash::hash hstate (vp1->block->index);
- int i;
tree phi1op;
tree type;
+ edge e;
+ edge_iterator ei;
/* If all PHI arguments are constants we need to distinguish
the PHI node via its type. */
type = vp1->type;
hstate.merge_hash (vn_hash_type (type));
- FOR_EACH_VEC_ELT (vp1->phiargs, i, phi1op)
+ FOR_EACH_EDGE (e, ei, vp1->block->preds)
{
+ /* Don't hash backedge values they need to be handled as VN_TOP
+ for optimistic value-numbering. */
+ if (e->flags & EDGE_DFS_BACK)
+ continue;
+
+ phi1op = vp1->phiargs[e->dest_idx];
if (phi1op == VN_TOP)
continue;
inchash::add_expr (phi1op, hstate);
@@ -2718,16 +2725,18 @@ vn_phi_lookup (gimple phi)
{
vn_phi_s **slot;
struct vn_phi_s vp1;
- unsigned i;
+ edge e;
+ edge_iterator ei;
shared_lookup_phiargs.truncate (0);
+ shared_lookup_phiargs.safe_grow (gimple_phi_num_args (phi));
/* Canonicalize the SSA_NAME's to their value number. */
- for (i = 0; i < gimple_phi_num_args (phi); i++)
+ FOR_EACH_EDGE (e, ei, gimple_bb (phi)->preds)
{
- tree def = PHI_ARG_DEF (phi, i);
+ tree def = PHI_ARG_DEF_FROM_EDGE (phi, e);
def = TREE_CODE (def) == SSA_NAME ? SSA_VAL (def) : def;
- shared_lookup_phiargs.safe_push (def);
+ shared_lookup_phiargs[e->dest_idx] = def;
}
vp1.type = TREE_TYPE (gimple_phi_result (phi));
vp1.phiargs = shared_lookup_phiargs;
@@ -2751,15 +2760,18 @@ vn_phi_insert (gimple phi, tree result)
{
vn_phi_s **slot;
vn_phi_t vp1 = current_info->phis_pool->allocate ();
- unsigned i;
vec args = vNULL;
+ edge e;
+ edge_iterator ei;
+
+ args.safe_grow (gimple_phi_num_args (phi));
/* Canonicalize the SSA_NAME's to their value number. */
- for (i = 0; i < gimple_phi_num_args (phi); i++)
+ FOR_EACH_EDGE (e, ei, gimple_bb (phi)->preds)
{
- tree def = PHI_ARG_DEF (phi, i);
+ tree def = PHI_ARG_DEF_FROM_EDGE (phi, e);
def = TREE_CODE (def) == SSA_NAME ? SSA_VAL (def) : def;
- args.safe_push (def);
+ args[e->dest_idx] = def;
}
vp1->value_id = VN_INFO (result)->value_id;
vp1->type = TREE_TYPE (gimple_phi_result (phi));
@@ -3244,28 +3256,23 @@ visit_phi (gimple phi)
if (def == VN_TOP)
continue;
if (sameval == VN_TOP)
+ sameval = def;
+ else if (!expressions_equal_p (def, sameval))
{
- sameval = def;
- }
- else
- {
- if (!expressions_equal_p (def, sameval))
- {
- allsame = false;
- break;
- }
+ allsame = false;
+ break;
}
}
- /* If all value numbered to the same value, the phi node has that
- value. */
- if (allsame)
-return set_ssa_val_to (PHI_RESULT (phi), sameval);
-
- /* Otherwise, see if it is equivalent to a phi node in this block. */
+ /* First see if it is equivalent to a phi node in this block. We prefer
+ this as it allows IV elimination - see PRs 66502 and 67167. */
result = vn_phi_lookup (phi);
if (result)
changed = set_ssa_val_to (PHI_RESULT (phi), result);
+ /* Otherwise all value numbered to the same value, the phi node has that
+ value. */
+ else if (allsame)
+changed = set_ssa_val_to (PHI_RESULT (phi), sameval);
else
{
vn_phi_insert (phi, PHI_RESULT (phi));
@@ -4155,6 +4162,8 @@ init_scc_vn (void)
int *rpo_numbers_temp;
calculate_dominance_info (CDI_DOMINATORS);
+ mark_dfs_back_edges ();
+
sccstack.create (0);
constant_to_value_id = new hash_table (23);
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-46.c
Re: [PATCH] Fix PRs 66502 and 67167
Richard Biener writes: > Given there is now PR67167 I am going forward with the earlier posted > patch to switch SCCVN to PHI elimination in favor of another PHI > (to remove IVs) rather than in favor of its only executable edge value. > > I still see no way to capture both cases without detecting the choice > and re-numbering the SCC twice, eventually choosing the "better" outcome. > And then the situation where both cases happen in the same SCC is not > handled either. > > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk. > > Richard. > > 2015-08-13 Richard Biener > > PR tree-optimization/66502 > PR tree-optimization/67167 > * tree-ssa-sccvn.c (vn_phi_compute_hash): Do not include > backedge arguments. > (vn_phi_lookup): Adjust. > (vn_phi_insert): Likewise. > (visit_phi): Prefer to value-number to another PHI node > over value-numbering to a PHI argument. > (init_scc_vn): Mark DFS back edges. Richard, I suspect this patch r226850 caused internal compiler error on ./gcc/testsuite/gcc.c-torture/compile/20121027-1.c on arm-non-eabi. The ICE gone away if I revert this patch. it can be easily reproduced by the following command. -mfpu and -mfloat are necessary. ./cc1 -O3 -nostdinc 20121027-1.c -march=armv8-a -mthumb -mfpu=crypto-neon-fp-armv8 -mfloat-abi=hard cc1 is generated from ../gcc/configure --target=arm-none-eabi --enable-languages=c,c++ do you mind to have a look? Thanks. -- Regards, Jiong
Re: [PATCH] Fix PRs 66502 and 67167
On Fri, 21 Aug 2015, Jiong Wang wrote: > > Richard Biener writes: > > > Given there is now PR67167 I am going forward with the earlier posted > > patch to switch SCCVN to PHI elimination in favor of another PHI > > (to remove IVs) rather than in favor of its only executable edge value. > > > > I still see no way to capture both cases without detecting the choice > > and re-numbering the SCC twice, eventually choosing the "better" outcome. > > And then the situation where both cases happen in the same SCC is not > > handled either. > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk. > > > > Richard. > > > > 2015-08-13 Richard Biener > > > > PR tree-optimization/66502 > > PR tree-optimization/67167 > > * tree-ssa-sccvn.c (vn_phi_compute_hash): Do not include > > backedge arguments. > > (vn_phi_lookup): Adjust. > > (vn_phi_insert): Likewise. > > (visit_phi): Prefer to value-number to another PHI node > > over value-numbering to a PHI argument. > > (init_scc_vn): Mark DFS back edges. > > Richard, > > I suspect this patch r226850 caused internal compiler error on > ./gcc/testsuite/gcc.c-torture/compile/20121027-1.c on arm-non-eabi. > The ICE gone away if I revert this patch. > > it can be easily reproduced by the following command. -mfpu and > -mfloat are necessary. > > ./cc1 -O3 -nostdinc 20121027-1.c -march=armv8-a -mthumb > -mfpu=crypto-neon-fp-armv8 -mfloat-abi=hard > > cc1 is generated from > > ../gcc/configure --target=arm-none-eabi --enable-languages=c,c++ > > do you mind to have a look? I see the following ICE: t.c:13:1: internal compiler error: in decompose_normal_address, at rtlanal.c:6090 } ^ 0xc94a37 decompose_normal_address /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6090 0xc94d25 decompose_address(address_info*, rtx_def**, machine_mode, unsigned char, rtx_code) /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6167 0xc94dc3 decompose_mem_address(address_info*, rtx_def*) /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6187 0xb61149 process_address_1 /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:2867 0xb61c4e process_address /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3124 0xb62607 curr_insn_transform /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3419 0xb65250 lra_constraints(bool) /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:4421 that looks like a latent issue to me in an area of GCC I am not familiar with. I suggest to open a bugreport and CC Vladimir. The r226850 change caused us to eliminate an induction variable early (I suspect IVOPTs would have done this later anyway, but I did not verify that): Replaced redundant PHI node defining bl_2 with c_1 Replaced c_1 + 1 with bl_15 in all uses of c_16 = c_1 + 1; Removing dead stmt c_16 = c_1 + 1; Removing dead stmt bl_2 = PHI <0(2), bl_15(3)> Thanks, Richard. > Thanks. > -- Richard Biener SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
Re: [PATCH] Fix PRs 66502 and 67167
Richard Biener writes: > I see the following ICE: > > t.c:13:1: internal compiler error: in decompose_normal_address, at > rtlanal.c:6090 > } > ^ > 0xc94a37 decompose_normal_address > /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6090 > 0xc94d25 decompose_address(address_info*, rtx_def**, machine_mode, > unsigned char, rtx_code) > /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6167 > 0xc94dc3 decompose_mem_address(address_info*, rtx_def*) > /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6187 > 0xb61149 process_address_1 > /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:2867 > 0xb61c4e process_address > /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3124 > 0xb62607 curr_insn_transform > /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3419 > 0xb65250 lra_constraints(bool) > /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:4421 > > that looks like a latent issue to me in an area of GCC I am not > familiar with. I suggest to open a bugreport and CC Vladimir. Thanks for the info. Done https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67305 > > The r226850 change caused us to eliminate an induction variable > early (I suspect IVOPTs would have done this later anyway, but > I did not verify that): > > Replaced redundant PHI node defining bl_2 with c_1 > Replaced c_1 + 1 with bl_15 in all uses of c_16 = c_1 + 1; > Removing dead stmt c_16 = c_1 + 1; > Removing dead stmt bl_2 = PHI <0(2), bl_15(3)> > > Thanks, > Richard. > >> Thanks. >> -- Regards, Jiong
Re: Re: [PATCH] Fix PRs 66502 and 67167
On 21/08/15 10:47, Jiong Wang wrote: Richard Biener writes: I see the following ICE: t.c:13:1: internal compiler error: in decompose_normal_address, at rtlanal.c:6090 } ^ 0xc94a37 decompose_normal_address /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6090 0xc94d25 decompose_address(address_info*, rtx_def**, machine_mode, unsigned char, rtx_code) /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6167 0xc94dc3 decompose_mem_address(address_info*, rtx_def*) /space/rguenther/tramp3d/trunk/gcc/rtlanal.c:6187 0xb61149 process_address_1 /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:2867 0xb61c4e process_address /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3124 0xb62607 curr_insn_transform /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:3419 0xb65250 lra_constraints(bool) /space/rguenther/tramp3d/trunk/gcc/lra-constraints.c:4421 that looks like a latent issue to me in an area of GCC I am not familiar with. I suggest to open a bugreport and CC Vladimir. Thanks for the info. Done https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67305 Richard, Though the ICE itself is caused by one latent bug in ARM backend (PR67305), while my further double check shows there is performance regression since this patch. The regression should have been caused by other gcc latent bugs in tree-vrp pass. Bugzilla created to track Thanks. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68234 Regards, Jiong The r226850 change caused us to eliminate an induction variable early (I suspect IVOPTs would have done this later anyway, but I did not verify that): Replaced redundant PHI node defining bl_2 with c_1 Replaced c_1 + 1 with bl_15 in all uses of c_16 = c_1 + 1; Removing dead stmt c_16 = c_1 + 1; Removing dead stmt bl_2 = PHI <0(2), bl_15(3)> Thanks, Richard. Thanks.
