Re: [PATCH] A jump threading opportunity for condition branch

[Richard Biener <rguenther@suse.de>]

On May 29, 2019 10:18:01 PM GMT+02:00, Jeff Law <law@redhat.com> wrote:
>On 5/23/19 6:11 AM, Richard Biener wrote:
>> On Thu, 23 May 2019, Jiufu Guo wrote:
>> 
>>> Hi,
>>>
>>> Richard Biener <rguenther@suse.de> writes:
>>>
>>>> On Tue, 21 May 2019, Jiufu Guo wrote:
>
>>>>> +    }
>>>>> +
>>>>> +  if (TREE_CODE_CLASS (gimple_assign_rhs_code (def)) !=
>tcc_comparison)
>>>>> +    return false;
>>>>> +
>>>>> +  /* Check if phi's incoming value is defined in the incoming
>basic_block.  */
>>>>> +  edge e = gimple_phi_arg_edge (phi, index);
>>>>> +  if (def->bb != e->src)
>>>>> +    return false;
>>>> why does this matter?
>>>>
>>> Through preparing pathes and duplicating block, this transform can
>also
>>> help to combine a cmp in previous block and a gcond in current
>block.
>>> "if (def->bb != e->src)" make sure the cmp is define in the incoming
>>> block of the current; and then combining "cmp with gcond" is safe. 
>If
>>> the cmp is defined far from the incoming block, it would be hard to
>>> achieve the combining, and the transform may not needed.
>> We're in SSA form so the "combining" doesn't really care where the
>> definition comes from.
>Combining doesn't care, but we need to make sure the copy of the
>conditional ends up in the right block since it wouldn't necessarily be
>associated with def->bb anymore.  But I'd expect the sinking pass to
>make this a non-issue in practice anyway.
>
>> 
>>>>> +
>>>>> +  if (!single_succ_p (def->bb))
>>>>> +    return false;
>>>> Or this?  The actual threading will ensure this will hold true.
>>>>
>>> Yes, other thread code check this and ensure it to be true, like
>>> function thread_through_normal_block. Since this new function is
>invoked
>>> outside thread_through_normal_block, so, checking single_succ_p is
>also
>>> needed for this case.
>> I mean threading will isolate the path making this trivially true.
>> It's also no requirement for combining, in fact due to the single-use
>> check the definition can be sinked across the edge already (if
>> the edges dest didn't have multiple predecessors which this threading
>> will fix as well).
>I don't think so.  The CMP source block could end with a call and have
>an abnormal edge (for example).  We can't put the copied conditional
>before the call and putting it after the call essentially means
>creating
>a new block.
>
>The CMP source block could also end with a conditional.  Where do we
>put
>the one we want to copy into the CMP source block in that case? :-)
>
>This is something else we'd want to check if we ever allowed the the
>CMP
>defining block to not be the immediate predecessor of the conditional
>jump block.  If we did that we'd need to validate that the block where
>we're going to insert the copy of the jump has a single successor.

But were just isolating a path here. The actual combine job is left to followup cleanups. 

Richard. 

>
>Jeff