Re: [PATCH PR81832]Skip copying loop header if inner loop is distributed

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Tue, Aug 15, 2017 at 6:33 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Tue, Aug 15, 2017 at 11:28 AM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>> Hi,
>>> This patch fixes PR81832.  Root cause for the ICE is:
>>>   1) Loop has distributed inner loop.
>>>   2) The guarding function call IFN_LOOP_DIST_CALL happens to be in loop's header.
>>>   3) IFN_LOOP_DIST_CALL (int loop's header) is duplicated by pass_ch_vect thus
>>>      not eliminated.
>>>
>>> Given pass_ch_vect copies loop header to enable more vectorization, we should
>>> skip loop in this case because distributed inner loop means this loop can not
>>> be vectorized anyway.  One point to mention is name inner_loop_distributed_p
>>> is a little misleading.  The name indicates that each basic block is checked,
>>> but the patch only checks loop's header for simplicity/efficiency's purpose.
>>> Any comment?
>>
>> My comment would be to question pass_ch_vect placement -- what was the
>> reason to place it so late?
>>
>> I also see GRAPHITE runs inbetween loop distribution and vectorization --
>> what prevents GRAPHITE from messing up things here?  Or autopar?
>>
>> The patch itself shows should_duplicate_loop_header_p should
>> handle this IFN specially (somehow all IFNs are considered "inexpensive").
>>
>> So can you please adjust should_duplicate_loop_header_p instead and/or
>> gimple_inexpensive_call_p?  Since we have IFNs for stuff like EXP10 I'm not
>> sure if that default is so good.
>
> I think the default itself is OK: we only use IFNs for libm functions
> like exp10 if an underlying optab exists (unlike __builtin_exp10, which
> is useful as the non-errno setting version of exp10), so the target must
> have something that it thinks is worth open-coding.  Also, we currently
> treat all MD built-ins as "simple" and thus "inexpensive", so the IFN
> handling is consistent with that.
>
> Maybe there are some IFNs that are worth special-casing as expensive,
> but IMO doing that to solve this problem would be a bit hacky.  It seems
> like "inexpensive" should be more of a cost thing than a correctness thing.
Hi all,
This is updated patch.  It only adds a single check on IFN_LOOP_DIST_ALIAS
in function should_duplicate_loop_header_p.  As for gimple_inexpensive_call_p,
I think it's natural to consider functions like IFN_LOOP_VECTORIZED and
IFN_LOOP_DIST_ALIAS as expensive because they are only meant to indicate
temporary arrangement of optimizations and are never used in code generation.
I will send a standalone patch for that.  Another thing is this patch
doesn't check
IFN_LOOP_VECTORIZED because it's impossible to have it with current order
of passes.
Bootstrap and test ongoing.  Further comments?

Thanks,
bin
2017-08-15  Bin Cheng  <bin.cheng@arm.com>

    PR tree-optimization/81832
    * tree-ssa-loop-ch.c (should_duplicate_loop_header_p): Don't
    copy loop header which has IFN_LOOP_DIST_ALIAS call.

gcc/testsuite
2017-08-15  Bin Cheng  <bin.cheng@arm.com>

    PR tree-optimization/81832
    * gcc.dg/tree-ssa/pr81832.c: New test.

>
> Thanks,
> Richard
>
>> Thanks,
>> Richard.
>>
>>> Bootstrap and test on x86_64.
>>>
>>> Thanks,
>>> bin
>>> 2017-08-15  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR tree-optimization/81832
>>>         * tree-ssa-loop-ch.c (inner_loop_distributed_p): New function.
>>>         (pass_ch_vect::process_loop_p): Call above function.
>>>
>>> gcc/testsuite
>>> 2017-08-15  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         PR tree-optimization/81832
>>>         * gcc.dg/tree-ssa/pr81832.c: New test.

Attachment: pr81832-20170816.txt
Description: Text document