On Fri, Apr 22, 2016 at 11:47 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Fri, Apr 22, 2016 at 12:33 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>> On Fri, Apr 22, 2016 at 11:25 AM, Richard Biener
>> <richard.guent...@gmail.com> wrote:
>>> On Fri, Apr 22, 2016 at 12:07 PM, Bin Cheng <bin.ch...@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.
>
Hi Richard,
Here is the updated patch. It also fixes PR70771 & PR70775. Root
cause for the ICE is in the fix to PR70725 because it forgot to
release single-argument PHI nodes after replacing uses. In
combine_blocks, these PHIs are removed from basic block but are still
live in IR. As a result, the ssa def/use chain for these PHIs are in
broken state, thus ICE is triggered whenever ssa use list is
accessed..
In this updated patch, I made below change to update virtual ssa
unconditionally. With this change, we don't need to handle virtual
PHIs explicitly, and single-argument PHI related code in fix to
PR70725 can also be removed.
@@ -2808,11 +2758,8 @@ tree_if_conversion (struct loop *loop)
}
todo |= TODO_cleanup_cfg;
- if (any_mask_load_store)
- {
- mark_virtual_operands_for_renaming (cfun);
- todo |= TODO_update_ssa_only_virtuals;
- }
+ mark_virtual_operands_for_renaming (cfun);
+ todo |= TODO_update_ssa_only_virtuals;
cleanup:
if (ifc_bbs)
BTW, it may be possible to only update affected PHIs using
mark_virtual_phi_result_for_renaming. This patch didn't do that since
the update is done once per function anyway.
Bootstrap and test on x86_64, is it OK?
Thanks,
bin
2016-04-25 Bin Cheng <bin.ch...@arm.com>
PR tree-optimization/70771
PR tree-optimization/70775
* 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.
(predicate_all_scalar_phis): Delete code handling single-argument
PHIs.
(tree_if_conversion): Mark and update virtual SSA.
gcc/testsuite/ChangeLog
2016-04-25 Bin Cheng <bin.ch...@arm.com>
PR tree-optimization/70771
PR tree-optimization/70775
* gcc.dg/pr70771.c: New test.
* gcc.dg/pr70771.c: New test.
Index: gcc/testsuite/gcc.dg/pr70771.c
===================================================================
--- gcc/testsuite/gcc.dg/pr70771.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr70771.c (working copy)
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+int a, b, c, d;
+
+static void
+fn1 ()
+{
+ for (b = 0; b < 1; b++)
+ for (c = 0; c < 1; c++)
+ {
+ if (a)
+ break;
+ b = 1;
+ }
+ for (;;)
+ ;
+}
+
+int
+main ()
+{
+ if (d)
+ fn1 ();
+ return 0;
+}
Index: gcc/testsuite/gcc.dg/pr70775.c
===================================================================
--- gcc/testsuite/gcc.dg/pr70775.c (revision 0)
+++ gcc/testsuite/gcc.dg/pr70775.c (working copy)
@@ -0,0 +1,34 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+struct S
+{
+ int f1;
+ int f2;
+} a;
+
+int b, c, d, e;
+short f;
+
+int
+fn1 (int p1, unsigned p2)
+{
+ return p1 + p2;
+}
+
+void
+fn2 ()
+{
+ struct S g;
+ int h;
+ for (; c; c++)
+ for (f = -3; f < 3; f = fn1 (f, 8))
+ {
+ a.f1 = e;
+ if (b)
+ a = g;
+ else
+ for (; h; h++)
+ d = b;
+ }
+}
Index: gcc/tree-if-conv.c
===================================================================
--- gcc/tree-if-conv.c (revision 235371)
+++ 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;
}
@@ -1915,28 +1879,14 @@ predicate_all_scalar_phis (struct loop *loop)
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))
- {
- gphi *phi = phi_gsi.phi ();
- replace_uses_by (gimple_phi_result (phi),
- gimple_phi_arg_def (phi, 0));
- }
+ phi = phi_gsi.phi ();
+ predicate_scalar_phi (phi, &gsi);
+ release_phi_node (phi);
+ gsi_next (&phi_gsi);
}
- 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);
- }
- }
set_phi_nodes (bb, NULL);
}
@@ -2808,11 +2758,8 @@ tree_if_conversion (struct loop *loop)
}
todo |= TODO_cleanup_cfg;
- if (any_mask_load_store)
- {
- mark_virtual_operands_for_renaming (cfun);
- todo |= TODO_update_ssa_only_virtuals;
- }
+ mark_virtual_operands_for_renaming (cfun);
+ todo |= TODO_update_ssa_only_virtuals;
cleanup:
if (ifc_bbs)
