Jeff Law <l...@redhat.com> writes:
> On 5/23/19 6:11 AM, Richard Biener wrote:
>> On Thu, 23 May 2019, Jiufu Guo wrote:
>>
>>> Hi,
>>>
>>> Richard Biener <rguent...@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.
OK, Adding single_succ_p (e->src) could make sure the copy jump is
insert to end of immediate predecessor, instead the define block of CMP,
if def->bb != e->src.
>
>
> Jeff
