Re: [PATCH 1/2] Introduce prefetch-minimum stride option

["H.J. Lu" <hjl.tools@gmail.com>]

On Tue, May 22, 2018 at 11:55 AM, Luis Machado <luis.machado@linaro.org> wrote:
>
>
> On 05/16/2018 08:18 AM, Luis Machado wrote:
>>
>>
>>
>> On 05/16/2018 06:08 AM, Kyrill Tkachov wrote:
>>>
>>>
>>> On 15/05/18 12:12, Luis Machado wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 05/15/2018 06:37 AM, Kyrill Tkachov wrote:
>>>>>
>>>>> Hi Luis,
>>>>>
>>>>> On 14/05/18 22:18, Luis Machado wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Here's an updated version of the patch (now reverted) that addresses
>>>>>> the previous bootstrap problem (signedness and long long/int conversion).
>>>>>>
>>>>>> I've checked that it bootstraps properly on both aarch64-linux and
>>>>>> x86_64-linux and that tests look sane.
>>>>>>
>>>>>> James, would you please give this one a try to see if you can still
>>>>>> reproduce PR85682? I couldn't reproduce it in multiple attempts.
>>>>>>
>>>>>
>>>>> The patch doesn't hit the regressions in PR85682 from what I can see.
>>>>> I have a comment on the patch below.
>>>>>
>>>>
>>>> Great. Thanks for checking Kyrill.
>>>>
>>>>> --- a/gcc/tree-ssa-loop-prefetch.c
>>>>> +++ b/gcc/tree-ssa-loop-prefetch.c
>>>>> @@ -992,6 +992,23 @@ prune_by_reuse (struct mem_ref_group *groups)
>>>>>   static bool
>>>>>   should_issue_prefetch_p (struct mem_ref *ref)
>>>>>   {
>>>>> +  /* Some processors may have a hardware prefetcher that may conflict
>>>>> with
>>>>> +     prefetch hints for a range of strides.  Make sure we don't issue
>>>>> +     prefetches for such cases if the stride is within this particular
>>>>> +     range.  */
>>>>> +  if (cst_and_fits_in_hwi (ref->group->step)
>>>>> +      && abs_hwi (int_cst_value (ref->group->step)) <
>>>>> +      (HOST_WIDE_INT) PREFETCH_MINIMUM_STRIDE)
>>>>> +    {
>>>>>
>>>>> The '<' should go on the line below together with
>>>>> PREFETCH_MINIMUM_STRIDE.
>>>>
>>>>
>>>> I've fixed this locally now.
>>>
>>>
>>> Thanks. I haven't followed the patch in detail, are you looking for
>>> midend changes approval since the last version?
>>> Or do you need aarch64 approval?
>>
>>
>> The changes are not substantial, but midend approval i what i was aiming
>> at.
>>
>> Also the confirmation that PR85682 is no longer happening.
>
>
> James confirmed PR85682 is no longer reproducible with the updated patch and
> the bootstrap issue is fixed now. So i take it this should be OK to push to
> mainline?
>
> Also, i'd like to discuss the possibility of having these couple options
> backported to GCC 8. As is, the changes don't alter code generation by
> default, but they allow better tuning of the software prefetcher for targets
> that benefit from it.
>
> Maybe after letting the changes bake on mainline enough to be confirmed
> stable?

It breaks GCC bootstrap on i686:

../../src-trunk/gcc/tree-ssa-loop-prefetch.c: In function ‘bool
should_issue_prefetch_p(mem_ref*)’:
../../src-trunk/gcc/tree-ssa-loop-prefetch.c:1015:4: error: format
‘%ld’ expects argument of type ‘long int’, but argument 5 has type
‘long long int’ [-Werror=format=]
    "Step for reference %u:%u (%ld) is less than the mininum "
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    "required stride of %d\n",
    ~~~~~~~~~~~~~~~~~~~~~~~~~
    ref->group->uid, ref->uid, int_cst_value (ref->group->step),
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

-- 
H.J.