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?
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.ch...@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.
>
