Re: [PATCH] Reducing number of alias checks in vectorization.

[Dehao Chen <dehao at google dot com>]

On Wed, Oct 2, 2013 at 10:50 AM, Cong Hou <congh@google.com> wrote:
> On Tue, Oct 1, 2013 at 11:35 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Tue, Oct 01, 2013 at 07:12:54PM -0700, Cong Hou wrote:
>>> --- gcc/tree-vect-loop-manip.c (revision 202662)
>>> +++ gcc/tree-vect-loop-manip.c (working copy)
>>
>> Your mailer ate all the tabs, so the formatting of the whole patch
>> can't be checked.
>>
>
>
> I'll pay attention to this problem in my later patch submission.
>
>
>>> @@ -19,6 +19,10 @@ You should have received a copy of the G
>>>  along with GCC; see the file COPYING3.  If not see
>>>  <http://www.gnu.org/licenses/>.  */
>>>
>>> +#include <vector>
>>> +#include <utility>
>>> +#include <algorithm>
>>
>> Why?  GCC has it's vec.h vectors, why don't you use those?
>> There is even qsort method for you in there.  And for pairs, you can
>> easily just use structs with two members as structure elements in the
>> vector.
>>
>
>
> GCC is now restructured using C++ and STL is one of the most important
> part of C++. I am new to GCC community and more familiar to STL (and I
> think allowing STL in GCC could attract more new developers for GCC).
> I agree using GCC's vec can maintain a uniform style but STL is just
> so powerful and easy to use...
>
> I just did a search in GCC source tree and found <vector> is not used
> yet. I will change std::vector to GCC's vec for now (and also qsort),
> but am still wondering if one day GCC would accept STL.

I talked with Ian and Diego before, they are both OK to have STL in
GCC code as soon as you just use it for local data structure that does
not use gcc garbage collector.

STL can greatly simply source code (e.g. In
http://gcc.gnu.org/viewcvs/gcc?view=revision&revision=201615 STL
helped reduce auto-profile.c from 1721 LOC to 1371 LOC). STL could
also attract more C++ developers to GCC community.

Any comments?
Dehao

>
>
>>> +struct dr_addr_with_seg_len
>>> +{
>>> +  dr_addr_with_seg_len (data_reference* d, tree addr, tree off, tree len)
>>> +    : dr (d), basic_addr (addr), offset (off), seg_len (len) {}
>>> +
>>> +  data_reference* dr;
>>
>> Space should be before *, not after it.
>>
>>> +  if (TREE_CODE (p11.offset) != INTEGER_CST
>>> +      || TREE_CODE (p21.offset) != INTEGER_CST)
>>> +    return p11.offset < p21.offset;
>>
>> If offset isn't INTEGER_CST, you are comparing the pointer values?
>> That is never a good idea, then compilation will depend on how say address
>> space randomization randomizes virtual address space.  GCC needs to have
>> reproduceable compilations.
>
>
> I this scenario comparing pointers is safe. The sort is used to put
> together any two pairs of data refs which can be merged. For example,
> if we have (a, b) (a, c), (a, b+1), then after sorting them we should
> have either (a, b), (a, b+1), (a, c) or (a, c), (a, b), (a, b+1). We
> don't care the relative order of "non-mergable" dr pairs here. So
> although the sorting result may vary the final result we get should
> not change.
>
>
>>
>>> +  if (int_cst_value (p11.offset) != int_cst_value (p21.offset))
>>> +    return int_cst_value (p11.offset) < int_cst_value (p21.offset);
>>
>> This is going to ICE whenever the offsets wouldn't fit into a
>> HOST_WIDE_INT.
>>
>> I'd say you just shouldn't put into the vector entries where offset isn't
>> host_integerp, those would never be merged with other checks, or something
>> similar.
>
> Do you mean I should use widest_int_cst_value()? Then I will replace
> all int_cst_value() here with it. I also changed the type of "diff"
> variable into HOST_WIDEST_INT.
>
>
>
> Thank you very much for your comments!
>
> Cong
>
>
>
>>
>>         Jakub