On Mon, Apr 25, 2016 at 6:44 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
> 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?
Ok.
Thanks,
Richard.
> 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.
