On Fri, Jan 27, 2017 at 12:20 PM, Aldy Hernandez <al...@redhat.com> wrote:
> On 01/26/2017 07:29 AM, Richard Biener wrote:
>>
>> On Thu, Jan 26, 2017 at 1:04 PM, Aldy Hernandez <al...@redhat.com> wrote:
>>>
>>> On 01/24/2017 07:23 AM, Richard Biener wrote:
>
>
>> Your initial on-demand approach is fine to catch some of the cases but it
>> will not handle those for which we'd need post-dominance.
>>
>> I guess we can incrementally add that.
>
>
> No complaints from me.
>
> This is my initial on-demand approach, with a few fixes you've commented on
> throughout.
>
> As you can see, there is still an #if 0 wrt to using your suggested
> conservative handling of memory loads, which I'm not entirely convinced of,
> as it pessimizes gcc.dg/loop-unswitch-1.c. If you feel strongly about it, I
> can enable the code again.
It is really required -- fortunately loop-unswitch-1.c is one of the cases where
the post-dom / always-executed bits help . The comparison is inside the
loop header and thus always executed when the loop enters, so inserrting it
on the preheader edge is fine.
> Also, I enhanced gcc.dg/loop-unswitch-1.c to verify that we're actually
> unswitching something. It seems kinda silly that we have various unswitch
> tests, but we don't actually check whether we have unswitched anything.
Heh. It probably was added for an ICE...
> This test was the only one in the *unswitch*.c set that I saw was actually
> being unswitched. Of course, if you don't agree with my #if 0 above, I will
> remove this change to the test.
>
> Finally, are we even guaranteed to unswitch in loop-unswitch-1.c across
> architectures? If not, again, I can remove the one-liner.
I think so.
>
> How does this look for trunk?
With a unswitch-local solution I meant to not add new files but put the
defined_or_undefined class (well, or rather a single function...) into
tree-ssa-loop-unswitch.c.
@@ -138,7 +141,7 @@ tree_may_unswitch_on (basic_block bb, struct loop *loop)
{
/* Unswitching on undefined values would introduce undefined
behavior that the original program might never exercise. */
- if (ssa_undefined_value_p (use, true))
+ if (defined_or_undefined->is_maybe_undefined (use))
return NULL_TREE;
def = SSA_NAME_DEF_STMT (use);
def_bb = gimple_bb (def);
as I said, moving this (now more expensive check) after
if (def_bb
&& flow_bb_inside_loop_p (loop, def_bb))
return NULL_TREE;
this cheap check would be better. It should avoid 99% of all calls I bet.
You can recover the loop-unswitch-1.c case by passing down
the using stmt and checking its BB against loop_header (the only
block that we trivially know is always executed when entering the region).
Or do that check in the caller, like
if (bb != loop->header
&& ssa_undefined_value_p (use, true) /
defined_or_undefined->is_maybe_undefined (use))
+ gimple *def = SSA_NAME_DEF_STMT (t);
+
+ /* Check that all the PHI args are fully defined. */
+ if (gphi *phi = dyn_cast <gphi *> (def))
+ {
+ if (virtual_operand_p (PHI_RESULT (phi)))
+ continue;
You should never run into virtual operands (you only walk SSA_OP_USEs).
You can stop walking at stmts that dominate the region header,
like with
+ gimple *def = SSA_NAME_DEF_STMT (t);
/* Uses in stmts always executed when the region header
executes are fine. */
if (dominated_by_p (CDI_DOMINATORS, loop_header, gimple_bb (def)))
continue;
and the bail out for PARM_DECLs is wrong:
+ /* A PARM_DECL will not have an SSA_NAME_DEF_STMT. Parameters
+ get their initial value from function entry. */
+ if (SSA_NAME_VAR (t) && TREE_CODE (SSA_NAME_VAR (t)) == PARM_DECL)
+ return false;
needs to be a continue; rather than a return false.
Otherwise looks ok and sorry for the continuing delays in reviewing this...
Thanks,
Richard.
> Aldy
>
