On Wed, May 23, 2018 at 3:35 PM, H.J. Lu <hjl.to...@gmail.com> wrote:
> On Wed, May 23, 2018 at 3:29 PM, Luis Machado <luis.mach...@linaro.org> wrote:
>>
>>
>> On 05/23/2018 05:01 PM, H.J. Lu wrote:
>>>
>>> On Tue, May 22, 2018 at 11:55 AM, Luis Machado <luis.mach...@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),
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>
>> Sorry. Does the following fix it for i686?
>>
>
> Index: gcc/tree-ssa-loop-prefetch.c
> ===================================================================
> --- gcc/tree-ssa-loop-prefetch.c (revision 260625)
> +++ gcc/tree-ssa-loop-prefetch.c (working copy)
> @@ -1014,7 +1014,8 @@
> fprintf (dump_file,
> "Step for reference %u:%u (%ld) is less than the mininum "
> ^^^ Please use
> HOST_WIDE_INT_PRINT_DEC
> "required stride of %d\n",
> - ref->group->uid, ref->uid, int_cst_value (ref->group->step),
> + ref->group->uid, ref->uid,
> + (HOST_WIDE_INT) int_cst_value (ref->group->step),
> PREFETCH_MINIMUM_STRIDE);
>
>
Something like this.
--
H.J.
---
diff --git a/gcc/tree-ssa-loop-prefetch.c b/gcc/tree-ssa-loop-prefetch.c
index c3e7fd1e529..949a67f360e 100644
--- a/gcc/tree-ssa-loop-prefetch.c
+++ b/gcc/tree-ssa-loop-prefetch.c
@@ -1012,8 +1012,8 @@ should_issue_prefetch_p (struct mem_ref *ref)
{
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file,
- "Step for reference %u:%u (%ld) is less than the mininum "
- "required stride of %d\n",
+ "Step for reference %u:%u (" HOST_WIDE_INT_PRINT_C
+ ") is less than the mininum required stride of %d\n",
ref->group->uid, ref->uid, int_cst_value (ref->group->step),
PREFETCH_MINIMUM_STRIDE);
return false;
