| Message ID | DB5PR08MB114425157CECF7E7DF230F12E76F0@DB5PR08MB1144.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 22, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: > Hi, > Tree if-conv has below code checking on virtual PHI nodes in if_convertible__phi_p: > > if (any_mask_load_store) > return true; > > /* When there were no if-convertible stores, check > that there are no memory writes in the branches of the loop to be > if-converted. */ > if (virtual_operand_p (gimple_phi_result (phi))) > { > imm_use_iterator imm_iter; > use_operand_p use_p; > > if (bb != loop->header) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, "Virtual phi not on loop->header.\n"); > return false; > } > > FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi)) > { > if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI > && USE_STMT (use_p) != phi) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, "Difficult to handle this virtual phi.\n"); > return false; > } > } > } > > After investigation, I think it's to bypass code in the form of: > > <bb header> > .MEM_2232 = PHI <.MEM_574(179), .MEM_1247(183)> // <----PHI_1 > ... > if (cond) > goto <bb 1> > else > goto <bb2> > > <bb 1>: //empty > <bb2>: > .MEM_1247 = PHI <.MEM_2232(180), .MEM_2232(181)> // <----PHI_2 > if (cond2) > goto <bb exit> > else > goto <bb latch> > > <bb latch>: > goto <bb header> > > Here PHI_2 can be degenerated and deleted. Furthermore, after propagating .MEM_2232 to .MEM_1247's uses, PHI_1 can also be degenerated and deleted in this case. These cases are bypassed because tree if-conv doesn't handle virtual PHI nodes during loop conversion (it only predicates scalar PHI nodes). Of course this results in loops not converted, and not vectorized. > This patch firstly deletes the aforementioned checking code, then adds code handling such virtual PHIs during conversion. The use of `any_mask_load_store' now is less ambiguous with this change, which allows further cleanups and patches fixing PR56541. > BTW, I think the newly fix at PR70725 on PHI nodes with only one argument is a special case covered by this change too. Unfortunately I can't use replace_uses_by because I need to handle PHIs at use point after replacing too. This doesn't really matter since we only care about virtual PHI, it's not possible to be used by anything other than IR itself. > Bootstrap and test on x86_64 and AArch64, is it OK if no regressions? Doesn't this undo my fix for degenerate non-virtual PHIs? I believe we can just drop virtual PHIs and rely on if (any_mask_load_store) { mark_virtual_operands_for_renaming (cfun); todo |= TODO_update_ssa_only_virtuals; } re-creating them from scratch. To do better than that we'd simply re-assign virtuals in combine_blocks in the new order (if there's any DEF, use the headers virtual def as first live vuse, assign that to any stmt with a vuse until you hit one with a vdef, then make that one life). Richard. > Thanks, > bin > > 2016-04-22 Bin Cheng <bin.cheng@arm.com> > > * tree-if-conv.c (if_convertible_phi_p): Remove check on special > virtual PHI nodes. Delete parameter. > (if_convertible_loop_p_1): Delete argument to above function. > (degenerate_virtual_phi): New function. > (predicate_all_scalar_phis): Rename to ... > (process_all_phis): ... here. Call degenerate_virtual_phi to > handle virtual PHIs. > (combine_blocks): Call renamed function. >
On Fri, Apr 22, 2016 at 11:25 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Apr 22, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >> Hi, >> Tree if-conv has below code checking on virtual PHI nodes in if_convertible__phi_p: >> >> if (any_mask_load_store) >> return true; >> >> /* When there were no if-convertible stores, check >> that there are no memory writes in the branches of the loop to be >> if-converted. */ >> if (virtual_operand_p (gimple_phi_result (phi))) >> { >> imm_use_iterator imm_iter; >> use_operand_p use_p; >> >> if (bb != loop->header) >> { >> if (dump_file && (dump_flags & TDF_DETAILS)) >> fprintf (dump_file, "Virtual phi not on loop->header.\n"); >> return false; >> } >> >> FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi)) >> { >> if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI >> && USE_STMT (use_p) != phi) >> { >> if (dump_file && (dump_flags & TDF_DETAILS)) >> fprintf (dump_file, "Difficult to handle this virtual phi.\n"); >> return false; >> } >> } >> } >> >> After investigation, I think it's to bypass code in the form of: >> >> <bb header> >> .MEM_2232 = PHI <.MEM_574(179), .MEM_1247(183)> // <----PHI_1 >> ... >> if (cond) >> goto <bb 1> >> else >> goto <bb2> >> >> <bb 1>: //empty >> <bb2>: >> .MEM_1247 = PHI <.MEM_2232(180), .MEM_2232(181)> // <----PHI_2 >> if (cond2) >> goto <bb exit> >> else >> goto <bb latch> >> >> <bb latch>: >> goto <bb header> >> >> Here PHI_2 can be degenerated and deleted. Furthermore, after propagating .MEM_2232 to .MEM_1247's uses, PHI_1 can also be degenerated and deleted in this case. These cases are bypassed because tree if-conv doesn't handle virtual PHI nodes during loop conversion (it only predicates scalar PHI nodes). Of course this results in loops not converted, and not vectorized. >> This patch firstly deletes the aforementioned checking code, then adds code handling such virtual PHIs during conversion. The use of `any_mask_load_store' now is less ambiguous with this change, which allows further cleanups and patches fixing PR56541. >> BTW, I think the newly fix at PR70725 on PHI nodes with only one argument is a special case covered by this change too. Unfortunately I can't use replace_uses_by because I need to handle PHIs at use point after replacing too. This doesn't really matter since we only care about virtual PHI, it's not possible to be used by anything other than IR itself. >> Bootstrap and test on x86_64 and AArch64, is it OK if no regressions? > > Doesn't this undo my fix for degenerate non-virtual PHIs? No, since we already support degenerate non-virtual PHIs in predicate_scalar_phi, your fix is also for virtual PHIs handling. > > I believe we can just drop virtual PHIs and rely on > > if (any_mask_load_store) > { > mark_virtual_operands_for_renaming (cfun); > todo |= TODO_update_ssa_only_virtuals; > } > > re-creating them from scratch. To do better than that we'd simply I tried this, simply enable above code for all cases can't resolve verify_ssa issue. I haven't look into the details, looks like ssa def-use chain is corrupted in if-conversion if we don't process it explicitly. Maybe it's possible along with your below suggestions, but we need to handle uses outside of loop too. Thanks, bin > re-assign virtuals > in combine_blocks in the new order (if there's any DEF, use the > headers virtual def > as first live vuse, assign that to any stmt with a vuse until you hit > one with a vdef, > then make that one life). > > Richard. > >> Thanks, >> bin >> >> 2016-04-22 Bin Cheng <bin.cheng@arm.com> >> >> * tree-if-conv.c (if_convertible_phi_p): Remove check on special >> virtual PHI nodes. Delete parameter. >> (if_convertible_loop_p_1): Delete argument to above function. >> (degenerate_virtual_phi): New function. >> (predicate_all_scalar_phis): Rename to ... >> (process_all_phis): ... here. Call degenerate_virtual_phi to >> handle virtual PHIs. >> (combine_blocks): Call renamed function. >>
On Fri, Apr 22, 2016 at 12:33 PM, Bin.Cheng <amker.cheng@gmail.com> wrote: > On Fri, Apr 22, 2016 at 11:25 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Fri, Apr 22, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>> Hi, >>> Tree if-conv has below code checking on virtual PHI nodes in if_convertible__phi_p: >>> >>> if (any_mask_load_store) >>> return true; >>> >>> /* When there were no if-convertible stores, check >>> that there are no memory writes in the branches of the loop to be >>> if-converted. */ >>> if (virtual_operand_p (gimple_phi_result (phi))) >>> { >>> imm_use_iterator imm_iter; >>> use_operand_p use_p; >>> >>> if (bb != loop->header) >>> { >>> if (dump_file && (dump_flags & TDF_DETAILS)) >>> fprintf (dump_file, "Virtual phi not on loop->header.\n"); >>> return false; >>> } >>> >>> FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi)) >>> { >>> if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI >>> && USE_STMT (use_p) != phi) >>> { >>> if (dump_file && (dump_flags & TDF_DETAILS)) >>> fprintf (dump_file, "Difficult to handle this virtual phi.\n"); >>> return false; >>> } >>> } >>> } >>> >>> After investigation, I think it's to bypass code in the form of: >>> >>> <bb header> >>> .MEM_2232 = PHI <.MEM_574(179), .MEM_1247(183)> // <----PHI_1 >>> ... >>> if (cond) >>> goto <bb 1> >>> else >>> goto <bb2> >>> >>> <bb 1>: //empty >>> <bb2>: >>> .MEM_1247 = PHI <.MEM_2232(180), .MEM_2232(181)> // <----PHI_2 >>> if (cond2) >>> goto <bb exit> >>> else >>> goto <bb latch> >>> >>> <bb latch>: >>> goto <bb header> >>> >>> Here PHI_2 can be degenerated and deleted. Furthermore, after propagating .MEM_2232 to .MEM_1247's uses, PHI_1 can also be degenerated and deleted in this case. These cases are bypassed because tree if-conv doesn't handle virtual PHI nodes during loop conversion (it only predicates scalar PHI nodes). Of course this results in loops not converted, and not vectorized. >>> This patch firstly deletes the aforementioned checking code, then adds code handling such virtual PHIs during conversion. The use of `any_mask_load_store' now is less ambiguous with this change, which allows further cleanups and patches fixing PR56541. >>> BTW, I think the newly fix at PR70725 on PHI nodes with only one argument is a special case covered by this change too. Unfortunately I can't use replace_uses_by because I need to handle PHIs at use point after replacing too. This doesn't really matter since we only care about virtual PHI, it's not possible to be used by anything other than IR itself. >>> Bootstrap and test on x86_64 and AArch64, is it OK if no regressions? >> >> Doesn't this undo my fix for degenerate non-virtual PHIs? > No, since we already support degenerate non-virtual PHIs in > predicate_scalar_phi, your fix is also for virtual PHIs handling. Was it? I don't remember ;) I think it was for a non-virtual PHI. Anyway, you should see the PR70725 testcase fail again if not. >> >> I believe we can just drop virtual PHIs and rely on >> >> if (any_mask_load_store) >> { >> mark_virtual_operands_for_renaming (cfun); >> todo |= TODO_update_ssa_only_virtuals; >> } >> >> re-creating them from scratch. To do better than that we'd simply > I tried this, simply enable above code for all cases can't resolve > verify_ssa issue. I haven't look into the details, looks like ssa > def-use chain is corrupted in if-conversion if we don't process it > explicitly. Maybe it's possible along with your below suggestions, > but we need to handle uses outside of loop too. Yes. I don't like all the new code to deal with virtual PHIs when doing it correctly would also avoid the above virtual SSA update ... After all the above seems to work for the case of if-converted stores (which is where virtual PHIs appear as well, even not degenerate). So I don't see exactly how it would break in the other case. I suppose you may need to call mark_virtual_phi_result_for_renaming () on all virtual PHIs. Thanks, Richard. > Thanks, > bin >> re-assign virtuals >> in combine_blocks in the new order (if there's any DEF, use the >> headers virtual def >> as first live vuse, assign that to any stmt with a vuse until you hit >> one with a vdef, >> then make that one life). >> >> Richard. >> >>> Thanks, >>> bin >>> >>> 2016-04-22 Bin Cheng <bin.cheng@arm.com> >>> >>> * tree-if-conv.c (if_convertible_phi_p): Remove check on special >>> virtual PHI nodes. Delete parameter. >>> (if_convertible_loop_p_1): Delete argument to above function. >>> (degenerate_virtual_phi): New function. >>> (predicate_all_scalar_phis): Rename to ... >>> (process_all_phis): ... here. Call degenerate_virtual_phi to >>> handle virtual PHIs. >>> (combine_blocks): Call renamed function. >>>
Index: gcc/tree-if-conv.c =================================================================== --- gcc/tree-if-conv.c (revision 235359) +++ gcc/tree-if-conv.c (working copy) @@ -640,16 +640,11 @@ phi_convertible_by_degenerating_args (gphi *phi) PHI is not if-convertible if: - it has more than 2 arguments. - When we didn't see if-convertible stores, PHI is not - if-convertible if: - - a virtual PHI is immediately used in another PHI node, - - there is a virtual PHI in a BB other than the loop->header. When the aggressive_if_conv is set, PHI can have more than two arguments. */ static bool -if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi, - bool any_mask_load_store) +if_convertible_phi_p (struct loop *loop, basic_block bb, gphi *phi) { if (dump_file && (dump_flags & TDF_DETAILS)) { @@ -669,36 +664,6 @@ static bool } } - if (any_mask_load_store) - return true; - - /* When there were no if-convertible stores, check - that there are no memory writes in the branches of the loop to be - if-converted. */ - if (virtual_operand_p (gimple_phi_result (phi))) - { - imm_use_iterator imm_iter; - use_operand_p use_p; - - if (bb != loop->header) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, "Virtual phi not on loop->header.\n"); - return false; - } - - FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi)) - { - if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI - && USE_STMT (use_p) != phi) - { - if (dump_file && (dump_flags & TDF_DETAILS)) - fprintf (dump_file, "Difficult to handle this virtual phi.\n"); - return false; - } - } - } - return true; } @@ -1405,8 +1370,7 @@ if_convertible_loop_p_1 (struct loop *loop, gphi_iterator itr; for (itr = gsi_start_phis (bb); !gsi_end_p (itr); gsi_next (&itr)) - if (!if_convertible_phi_p (loop, bb, itr.phi (), - *any_mask_load_store)) + if (!if_convertible_phi_p (loop, bb, itr.phi ())) return false; } @@ -1682,6 +1646,58 @@ gen_phi_arg_condition (gphi *phi, vec<int> *occur, return cond; } +/* Degenerate virtual PHI node by propagating its operand to all uses + of its def. After degeneration, the PHI node becomes dead and is + removed. This function also tries to degenerate other virtual phi + nodes in LOOP after propagation. */ + +static void +degenerate_virtual_phi (struct loop *loop, gphi *phi) +{ + auto_vec<gphi *, 4> worklist; + + worklist.safe_push (phi); + while (worklist.length () > 0) + { + tree vop, vdef; + gimple *use_stmt; + imm_use_iterator iter; + gimple_stmt_iterator gsi; + + phi = worklist.pop (); + vop = degenerate_phi_result (phi); + gcc_assert (vop != NULL); + + /* Degenerate PHI by replacing def with op. */ + vdef = gimple_phi_result (phi); + FOR_EACH_IMM_USE_STMT (use_stmt, iter, vdef) + { + use_operand_p use_p; + gphi *use_phi = dyn_cast <gphi *> (use_stmt); + + /* Skip use in itself. */ + if (use_phi == phi) + continue; + + FOR_EACH_IMM_USE_ON_STMT (use_p, iter) + SET_USE (use_p, vop); + + update_stmt(use_stmt); + /* Check if new PHI node can be degenerated or not. */ + if (use_phi + && virtual_operand_p (gimple_phi_result (use_phi)) + && flow_bb_inside_loop_p (loop, gimple_bb (use_phi)) + && degenerate_phi_result (use_phi)) + worklist.safe_push (use_phi); + } + gsi = gsi_for_stmt (phi); + remove_phi_node (&gsi, true); + if (TREE_CODE (vop) == SSA_NAME + && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (vdef)) + SSA_NAME_OCCURS_IN_ABNORMAL_PHI (vop) = 1; + } +} + /* Replace a scalar PHI node with a COND_EXPR using COND as condition. This routine can handle PHI nodes with more than two arguments. @@ -1895,7 +1911,7 @@ predicate_scalar_phi (gphi *phi, gimple_stmt_itera LOOP->header block with conditional modify expressions. */ static void -predicate_all_scalar_phis (struct loop *loop) +process_all_phis (struct loop *loop) { basic_block bb; unsigned int orig_loop_num_nodes = loop->num_nodes; @@ -1915,27 +1931,19 @@ static void if (gsi_end_p (phi_gsi)) continue; - if (EDGE_COUNT (bb->preds) == 1) + gsi = gsi_after_labels (bb); + while (!gsi_end_p (phi_gsi)) { - /* Propagate degenerate PHIs. */ - for (phi_gsi = gsi_start_phis (bb); !gsi_end_p (phi_gsi); - gsi_next (&phi_gsi)) + phi = phi_gsi.phi (); + if (virtual_operand_p (gimple_phi_result (phi)) + && degenerate_phi_result (phi) != NULL) + degenerate_virtual_phi (loop, phi); + else { - gphi *phi = phi_gsi.phi (); - replace_uses_by (gimple_phi_result (phi), - gimple_phi_arg_def (phi, 0)); - } - } - else - { - gsi = gsi_after_labels (bb); - while (!gsi_end_p (phi_gsi)) - { - phi = phi_gsi.phi (); predicate_scalar_phi (phi, &gsi); release_phi_node (phi); - gsi_next (&phi_gsi); } + gsi_next (&phi_gsi); } set_phi_nodes (bb, NULL); @@ -2289,7 +2297,7 @@ combine_blocks (struct loop *loop, bool any_mask_l predicate_bbs (loop); remove_conditions_and_labels (loop); insert_gimplified_predicates (loop, any_mask_load_store); - predicate_all_scalar_phis (loop); + process_all_phis (loop); if (any_mask_load_store) predicate_mem_writes (loop);
Hi, Tree if-conv has below code checking on virtual PHI nodes in if_convertible__phi_p: if (any_mask_load_store) return true; /* When there were no if-convertible stores, check that there are no memory writes in the branches of the loop to be if-converted. */ if (virtual_operand_p (gimple_phi_result (phi))) { imm_use_iterator imm_iter; use_operand_p use_p; if (bb != loop->header) { if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, "Virtual phi not on loop->header.\n"); return false; } FOR_EACH_IMM_USE_FAST (use_p, imm_iter, gimple_phi_result (phi)) { if (gimple_code (USE_STMT (use_p)) == GIMPLE_PHI && USE_STMT (use_p) != phi) { if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, "Difficult to handle this virtual phi.\n"); return false; } } } After investigation, I think it's to bypass code in the form of: <bb header> .MEM_2232 = PHI <.MEM_574(179), .MEM_1247(183)> // <----PHI_1 ... if (cond) goto <bb 1> else goto <bb2> <bb 1>: //empty <bb2>: .MEM_1247 = PHI <.MEM_2232(180), .MEM_2232(181)> // <----PHI_2 if (cond2) goto <bb exit> else goto <bb latch> <bb latch>: goto <bb header> Here PHI_2 can be degenerated and deleted. Furthermore, after propagating .MEM_2232 to .MEM_1247's uses, PHI_1 can also be degenerated and deleted in this case. These cases are bypassed because tree if-conv doesn't handle virtual PHI nodes during loop conversion (it only predicates scalar PHI nodes). Of course this results in loops not converted, and not vectorized. This patch firstly deletes the aforementioned checking code, then adds code handling such virtual PHIs during conversion. The use of `any_mask_load_store' now is less ambiguous with this change, which allows further cleanups and patches fixing PR56541. BTW, I think the newly fix at PR70725 on PHI nodes with only one argument is a special case covered by this change too. Unfortunately I can't use replace_uses_by because I need to handle PHIs at use point after replacing too. This doesn't really matter since we only care about virtual PHI, it's not possible to be used by anything other than IR itself. Bootstrap and test on x86_64 and AArch64, is it OK if no regressions? Thanks, bin 2016-04-22 Bin Cheng <bin.cheng@arm.com> * tree-if-conv.c (if_convertible_phi_p): Remove check on special virtual PHI nodes. Delete parameter. (if_convertible_loop_p_1): Delete argument to above function. (degenerate_virtual_phi): New function. (predicate_all_scalar_phis): Rename to ... (process_all_phis): ... here. Call degenerate_virtual_phi to handle virtual PHIs. (combine_blocks): Call renamed function.