On Tue, Aug 15, 2017 at 6:33 PM, Richard Sandiford
<richard.sandif...@linaro.org> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>> On Tue, Aug 15, 2017 at 11:28 AM, Bin Cheng <bin.ch...@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.ch...@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.ch...@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.ch...@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.ch...@arm.com>
>>>
>>> PR tree-optimization/81832
>>> * gcc.dg/tree-ssa/pr81832.c: New test.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr81832.c
b/gcc/testsuite/gcc.dg/tree-ssa/pr81832.c
new file mode 100644
index 0000000..893124e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr81832.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O3" } */
+
+int a, b, *c;
+void d(void)
+{
+ int **e;
+ for(;;)
+ for(int f = 1; f <= 6; f++)
+ {
+ b = 0;
+ if(a)
+g:
+ while(a++);
+ if (**e);
+ else
+ {
+ *c = a;
+ goto g;
+ }
+ }
+}
diff --git a/gcc/tree-ssa-loop-ch.c b/gcc/tree-ssa-loop-ch.c
index 14cc6d8d..6bb0220 100644
--- a/gcc/tree-ssa-loop-ch.c
+++ b/gcc/tree-ssa-loop-ch.c
@@ -119,7 +119,10 @@ should_duplicate_loop_header_p (basic_block header, struct
loop *loop,
continue;
if (gimple_code (last) == GIMPLE_CALL
- && !gimple_inexpensive_call_p (as_a <gcall *> (last)))
+ && (!gimple_inexpensive_call_p (as_a <gcall *> (last))
+ /* IFN_LOOP_DIST_ALIAS means that inner loop is distributed
+ at current loop's header. Don't copy in this case. */
+ || gimple_call_internal_p (last, IFN_LOOP_DIST_ALIAS)))
{
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file,
