On Fri, Apr 22, 2016 at 11:25 AM, Richard Biener
<[email protected]> wrote:
> On Fri, Apr 22, 2016 at 12:07 PM, Bin Cheng <[email protected]> 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 <[email protected]>
>>
>> * 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.
>>
