On Mon, May 9, 2016 at 11:54 AM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Fri, May 6, 2016 at 11:42 AM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>> On Fri, May 6, 2016 at 10:40 AM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>> On Tue, May 3, 2016 at 11:08 AM, Richard Biener
>>> <richard.guent...@gmail.com> wrote:
>>>> On Tue, May 3, 2016 at 12:01 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>>>> On Mon, May 2, 2016 at 10:00 AM, Richard Biener
>>>>> <richard.guent...@gmail.com> wrote:
>>>>>> On Fri, Apr 29, 2016 at 5:05 PM, Bin.Cheng <amker.ch...@gmail.com> wrote:
>>>>>>> On Fri, Apr 29, 2016 at 12:16 PM, Richard Biener
>>>>>>> <richard.guent...@gmail.com> wrote:
>>>>>>>> On Thu, Apr 28, 2016 at 2:56 PM, Bin Cheng <bin.ch...@arm.com> wrote:
>>>>>>>>> Hi,
>>>>>>>>> Tree if-conversion sometimes cannot convert conditional array
>>>>>>>>> reference into unconditional one. Root cause is GCC conservatively
>>>>>>>>> assumes newly introduced array reference could be out of array bound
>>>>>>>>> and thus trapping. This patch improves the situation by proving the
>>>>>>>>> converted unconditional array reference is within array bound using
>>>>>>>>> loop niter information. To be specific, it checks every index of
>>>>>>>>> array reference to see if it's within bound in
>>>>>>>>> ifcvt_memrefs_wont_trap. This patch also factors out
>>>>>>>>> base_object_writable checking if the base object is writable or not.
>>>>>>>>> Bootstrap and test on x86_64 and aarch64, is it OK?
>>>>>>>>
>>>>>>>> I think you miss to handle the case optimally where the only
>>>>>>>> non-ARRAY_REF idx is the dereference of the
>>>>>>>> base-pointer for, say, p->a[i]. In this case we can use
>>>>>>>> base_master_dr to see if p is unconditionally dereferenced
>>>>>>> Yes, will pick up this case.
>>>>>>>
>>>>>>>> in the loop. You also fail to handle the case where we have
>>>>>>>> MEM_REF[&x].a[i] that is, you see a decl base.
>>>>>>> I am having difficulty in creating this case for ifcvt, any advices?
>>>>>>> Thanks.
>>>>>>
>>>>>> Sth like
>>>>>>
>>>>>> float a[128];
>>>>>> float foo (int n, int i)
>>>>>> {
>>>>>> return (*((float(*)[n])a))[i];
>>>>>> }
>>>>>>
>>>>>> should do the trick (w/o the component-ref). Any other type-punning
>>>>>> would do it, too.
>>>>>>
>>>>>>>> I suppose for_each_index should be fixed for this particular case (to
>>>>>>>> return true), same for TARGET_MEM_REF TMR_BASE.
>>>>>>>>
>>>>>>>> + /* The case of nonconstant bounds could be handled, but it would be
>>>>>>>> + complicated. */
>>>>>>>> + if (TREE_CODE (low) != INTEGER_CST || !integer_zerop (low)
>>>>>>>> + || !high || TREE_CODE (high) != INTEGER_CST)
>>>>>>>> + return false;
>>>>>>>> +
>>>>>>>>
>>>>>>>> handling of a non-zero but constant low bound is important - otherwise
>>>>>>>> all this is a no-op for Fortran. It
>>>>>>>> shouldn't be too difficult to handle after all. In fact I think your
>>>>>>>> code does handle it correctly already.
>>>>>>>>
>>>>>>>> + if (!init || TREE_CODE (init) != INTEGER_CST
>>>>>>>> + || !step || TREE_CODE (step) != INTEGER_CST || integer_zerop
>>>>>>>> (step))
>>>>>>>> + return false;
>>>>>>>>
>>>>>>>> step == 0 should be easy to handle as well, no? The index will simply
>>>>>>>> always be 'init' ...
>>>>>>>>
>>>>>>>> + /* In case the relevant bound of the array does not fit in type, or
>>>>>>>> + it does, but bound + step (in type) still belongs into the range
>>>>>>>> of the
>>>>>>>> + array, the index may wrap and still stay within the range of the
>>>>>>>> array
>>>>>>>> + (consider e.g. if the array is indexed by the full range of
>>>>>>>> + unsigned char).
>>>>>>>> +
>>>>>>>> + To make things simpler, we require both bounds to fit into type,
>>>>>>>> although
>>>>>>>> + there are cases where this would not be strictly necessary. */
>>>>>>>> + if (!int_fits_type_p (high, type) || !int_fits_type_p (low, type))
>>>>>>>> + return false;
>>>>>>>> +
>>>>>>>> + low = fold_convert (type, low);
>>>>>>>>
>>>>>>>> please use wide_int for all of this.
>>>>>>> Now I use wi:fits_to_tree_p instead of int_fits_type_p. But I am not
>>>>>>> sure what's the meaning by "handle "low = fold_convert (type, low);"
>>>>>>> related code in wide_int". Do you mean to use tree_int_cst_compare
>>>>>>> instead of tree_int_cst_compare in the following code?
>>>>>>
>>>>>> I don't think you need any kind of fits-to-type check here. You'd simply
>>>>>> use to_widest () when operating on / comparing with high/low.
>>>>> But what would happen if low/high and init/step are different in type
>>>>> sign-ness? Anything special I need to do before using wi::ltu_p or
>>>>> wi::lts_p directly?
>>>>
>>>> You want to use to_widest (min) which extends according to sign to
>>>> an "infinite" precision signed integer. So you can then use the new
>>>> operator< overloads as well.
>>>>
>>> Hi,
>>> Here is the updated patch. It includes below changes according to
>>> review comments:
>>>
>>> 1) It uses widest_int for all INTEGER_CST tree computations, which
>>> simplifies the patch a lot.
>>> 2) It covers array with non-zero low bound, which is important for Fortran.
>>> 3) It picks up a boundary case so that ifc-11.c/vect-23.c/vect-24.c
>>> can be handled.
>>> 4) It also checks within bound array reference inside a structure like
>>> p->a[i] by using base_master_dr in tree-if-conv.c so that ifc-12.c can
>>> be handled.
>>>
>>> It leaves two other review comments not addressed:
>>> 1) It doesn't handle array reference whose idx is a wrapping SCEV.
>>> Because only read is safe and vectorizer itself may be confused by it
>>> now.
>>> 2) It doesn't handle array reference in the form of
>>> "MEM[(float[0:(sizetype) ((long int) SAVE_EXPR <m.2> + -1)]
>>> *)&b][i_1];". To handle this case, existing code in
>>> array_at_struct_end_p as well as this patch need to be improved. I
>>> tend to handle it in an independent patch.
>>>
>>> With these changes, now cases pr61194.c and vect-23.c can be
>>> vectorized, I removed XFAIL from them. Also vect-mask-store-move-1.c
>>> is affected and not vectorized now, this is tricky and it exposes a
>>> known bug PR65206 in vectorizer's dependence analyzer. This should be
>>> handled independently too. Also gcc.dg/vect/vect-24.c now is XPASS on
>>> AArch64, but not x86_64. Root cause is dom2 jump threads first
>>> iteration of loop thus idx_within_array_bound is failed. I didn't
>>> check if jump threading is good in this case, either I remove the
>>> XFAIL mark. I tend to improve idx_within_array_bound by using VRP
>>> information in a following patch.
>>
>> Hmm, I accidentally included removal of XFAIL for
>> gcc.dg/vect/vect-24.c in the patch. Anyway, it can be included or
>> excluded wrto reviewers opinion.
>>
>> Thanks,
>> bin
>>> Otherwise bootstrap and test on x86_64 and AArch64 are fine. Is this
>>> version OK?
>
> Ok. Adjusting gcc.dg/vect/vect-24.c is your choice, if things change again
> with a followup you might want to defer the change to that.
Right. I looked into VRP for vect-24.c and found it's not that easy.
The range_info computed for the index is [1, 64] which is useless
because the array bound is [0, 63]. We need to improve VRP info to
[1, 63]. Another possible issue is tricky. In other optimizers we
may want to use control-flow sensitive VRP information. However in
tree-if-conversion, we need to get rid of VRP information coming from
the condition of if-statement that is being converted right now. It
doesn't matter for now, but it will if we compute flow-sensitive VRP
in the future.
For this case maybe it's worth investigating why dom2 threads the
first loop iteration. After all, it's a canonical loop, threading the
first iteration leaves 63 iterations, which isn't friendly at least to
both vectorizer and unroller.
I will leave gcc.dg/vect/vect-24.c as it is.
Thanks,
bin
>
> Thanks,
> Richard.
>
>>> Thanks,
>>> bin
>>>
>>> 2016-05-04 Bin Cheng <bin.ch...@arm.com>
>>>
>>> * tree-if-conv.c (tree-ssa-loop.h): Include header file.
>>> (tree-ssa-loop-niter.h): Ditto.
>>> (idx_within_array_bound, ref_within_array_bound): New functions.
>>> (ifcvt_memrefs_wont_trap): Check if array ref is within bound.
>>> Factor out check on writable base object to ...
>>> (base_object_writable): ... here.
>>>
>>> gcc/testsuite/ChangeLog
>>> 2016-05-04 Bin Cheng <bin.ch...@arm.com>
>>>
>>> * gcc.dg/tree-ssa/ifc-9.c: New test.
>>> * gcc.dg/tree-ssa/ifc-10.c: New test.
>>> * gcc.dg/tree-ssa/ifc-11.c: New test.
>>> * gcc.dg/tree-ssa/ifc-12.c: New test.
>>> * gcc.dg/vect/pr61194.c: Remove XFAIL.
>>> * gcc.dg/vect/vect-23.c: Remove XFAIL.
>>> * gcc.dg/vect/vect-mask-store-move-1.c: Revise test check.
