llvm dev - Aug 2017 - [ScalarEvolution][SCEV] no-wrap flags dependent on order of getSCEV() calls

> On Aug 14, 2017, at 7:35 AM, Geoff Berry <gberry at codeaurora.org>
wrote:
> 
> Hi Sanjoy,
> 
> [adding Adam since I believe he added the original FIXME to preserve SCEV
> in LoopDataPrefetch]
For record, that wasn’t me.  It was there from the beginning when Hal added the
PPC-specific pass.

Adam
> 
> On 8/14/2017 1:36 AM, Sanjoy Das wrote:
>> Hi Geoff,
>> On Wed, Aug 9, 2017 at 8:58 AM, Geoff Berry <gberry at
codeaurora.org> wrote:
>>> On 8/8/2017 8:38 PM, Sanjoy Das wrote:
>>>> 
>>>> Hi,
>>>> 
>>>> On Tue, Aug 8, 2017 at 12:58 PM, Friedman, Eli <efriedma at
codeaurora.org>
>>>> wrote:
>>>>> 
>>>>> Oh, I see... yes, we do stupid things involving mutating
NoWrap flags
>>>>> after
>>>>> a SCEV is created.  (grep for setNoWrapFlags in
ScalarEvolution.cpp.)
>>>> 
>>>> 
>>>> That's really a compile time hack -- we defer some
expensive tricks to
>>>> prove nsw/nuw on an add recurrences to when we've been
asked to
>>>> sign/zero extend said add recurrence.
>>> 
>>> 
>>> If that is the case, preserving SCEV analysis where we didn't
before should
>>> only
>>> result in later passes seeing more nsw/nuw flags, which should
theoretically
>>> be
>>> beneficial?
>> There are downsides to preserving SCEV for "too long" though
-- if you
>> simplified a loop in a way that SCEV can compute its trip count when
>> it couldn't before (or it could compute a trip count before, but
now
>> it can compute a more precise trip count) then invalidating the cache
>> can be an improvement.  However, in this case judicious use of
>> forgetLoop() (as opposed to re-constructing the whole of SCEV) will
>> also do the job.
> 
> In this particular case, neither of the passes that I'm marking as
newly
> preserving SCEV modify any loop structure or any instructions that should
> impact any of the existing SCEV values.  All they do is add prefetch
> instructions (and some new instructions to compute the addresses to
> prefetch) and annotate some load instructions with metadata.  That's
why
> I was surprised to see changes in generated code from marking them as
> preserving SCEV.
> 
> I've done some performance testing and didn't see any significant
changes
> in performance due to these minor SCEV changes, so I think I'm going to
> post a patch to enable SCEV preservation in these passes and move on.
> 
>> -- Sanjoy
>>>> I would be okay if you wanted to expose those tricks via a
helper in
>>>> SCEV that could be called independently of creating zero or
sign
>>>> extensions.  That way SCEV clients could trade-off compile time
for> more
>>>> precise information in a granular way.
>>> 
>>> 
>>> I'm not concerned with getting better SCEV results at the
moment, I just
>>> want to
>>> make sure that changing these passes to preserve SCEV analysis is
behaving
>>> as
>>> expected (which it sounds like it is).
>>> 
>>>> 
>>>> -- Sanjoy
>>>> 
>>>>> 
>>>>> -Eli
>>>>> 
>>>>> --
>>>>> Employee of Qualcomm Innovation Center, Inc.
>>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora
Forum, a
>>>>> Linux
>>>>> Foundation Collaborative Project
>>>>> 
>>> 
>>> --
>>> Geoff Berry
>>> Employee of Qualcomm Datacenter Technologies, Inc.
>>>  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>>> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code
>>> Aurora Forum, a Linux Foundation Collaborative Project.
> 
> -- 
> Geoff Berry
> Employee of Qualcomm Datacenter Technologies, Inc.
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20170814/51c582a5/attachment.html>

On 8/14/2017 6:52 PM, Adam Nemet wrote:
> 
>> On Aug 14, 2017, at 7:35 AM, Geoff Berry <gberry at codeaurora.org 
>> <mailto:gberry at codeaurora.org>> wrote:
>>
>> Hi Sanjoy,
>>
>> [adding Adam since I believe he added the original FIXME to preserve
SCEV
>> in LoopDataPrefetch]
> 
> For record, that wasn’t me.  It was there from the beginning when Hal 
> added the PPC-specific pass.
My mistake.  Hal, would you like to chime in on 
https://reviews.llvm.org/D36716
> Adam
> 
>>
>> On 8/14/2017 1:36 AM, Sanjoy Das wrote:
>>> Hi Geoff,
>>> On Wed, Aug 9, 2017 at 8:58 AM, Geoff Berry <gberry at
codeaurora.org
>>> <mailto:gberry at codeaurora.org>> wrote:
>>>> On 8/8/2017 8:38 PM, Sanjoy Das wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Tue, Aug 8, 2017 at 12:58 PM, Friedman, Eli 
>>>>> <efriedma at codeaurora.org <mailto:efriedma at
codeaurora.org>>
>>>>> wrote:
>>>>>>
>>>>>> Oh, I see... yes, we do stupid things involving
mutating NoWrap flags
>>>>>> after
>>>>>> a SCEV is created.  (grep for setNoWrapFlags in
ScalarEvolution.cpp.)
>>>>>
>>>>>
>>>>> That's really a compile time hack -- we defer some
expensive tricks to
>>>>> prove nsw/nuw on an add recurrences to when we've been
asked to
>>>>> sign/zero extend said add recurrence.
>>>>
>>>>
>>>> If that is the case, preserving SCEV analysis where we
didn't before
>>>> should
>>>> only
>>>> result in later passes seeing more nsw/nuw flags, which should 
>>>> theoretically
>>>> be
>>>> beneficial?
>>> There are downsides to preserving SCEV for "too long"
though -- if you
>>> simplified a loop in a way that SCEV can compute its trip count
when
>>> it couldn't before (or it could compute a trip count before,
but now
>>> it can compute a more precise trip count) then invalidating the
cache
>>> can be an improvement.  However, in this case judicious use of
>>> forgetLoop() (as opposed to re-constructing the whole of SCEV) will
>>> also do the job.
>>
>> In this particular case, neither of the passes that I'm marking as
newly
>> preserving SCEV modify any loop structure or any instructions that
should
>> impact any of the existing SCEV values.  All they do is add prefetch
>> instructions (and some new instructions to compute the addresses to
>> prefetch) and annotate some load instructions with metadata. 
That's why
>> I was surprised to see changes in generated code from marking them as
>> preserving SCEV.
>>
>> I've done some performance testing and didn't see any
significant changes
>> in performance due to these minor SCEV changes, so I think I'm
going to
>> post a patch to enable SCEV preservation in these passes and move on.
>>
>>> -- Sanjoy
>>>>> I would be okay if you wanted to expose those tricks via a
helper in
>>>>> SCEV that could be called independently of creating zero or
sign
>>>>> extensions.  That way SCEV clients could trade-off compile
time
>>>>> for> more
>>>>> precise information in a granular way.
>>>>
>>>>
>>>> I'm not concerned with getting better SCEV results at the
moment, I just
>>>> want to
>>>> make sure that changing these passes to preserve SCEV analysis
is
>>>> behaving
>>>> as
>>>> expected (which it sounds like it is).
>>>>
>>>>>
>>>>> -- Sanjoy
>>>>>
>>>>>>
>>>>>> -Eli
>>>>>>
>>>>>> --
>>>>>> Employee of Qualcomm Innovation Center, Inc.
>>>>>> Qualcomm Innovation Center, Inc. is a member of Code
Aurora Forum, a
>>>>>> Linux
>>>>>> Foundation Collaborative Project
>>>>>>
>>>>
>>>> --
>>>> Geoff Berry
>>>> Employee of Qualcomm Datacenter Technologies, Inc.
>>>>  Qualcomm Datacenter Technologies, Inc. as an affiliate of
Qualcomm
>>>> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of
the Code
>>>> Aurora Forum, a Linux Foundation Collaborative Project.
>>
>> --
>> Geoff Berry
>> Employee of Qualcomm Datacenter Technologies, Inc.
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
>> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the 
>> Code Aurora Forum, a Linux Foundation Collaborative Project.
> 
-- 
Geoff Berry
Employee of Qualcomm Datacenter Technologies, Inc.
  Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the Code 
Aurora Forum, a Linux Foundation Collaborative Project.

On 08/14/2017 10:28 PM, Geoff Berry wrote:
>
>
> On 8/14/2017 6:52 PM, Adam Nemet wrote:
>>
>>> On Aug 14, 2017, at 7:35 AM, Geoff Berry <gberry at
codeaurora.org
>>> <mailto:gberry at codeaurora.org>> wrote:
>>>
>>> Hi Sanjoy,
>>>
>>> [adding Adam since I believe he added the original FIXME to
preserve
>>> SCEV
>>> in LoopDataPrefetch]
>>
>> For record, that wasn’t me.  It was there from the beginning when Hal 
>> added the PPC-specific pass.
>
> My mistake.  Hal, would you like to chime in on 
> https://reviews.llvm.org/D36716 '
I don't have anything to add in particular. As I recall, marking this as 
preserved used to cause crashes later in the pipeline in some cases. 
Might as well see if it's still true.

Thanks again,
Hal
>
>> Adam
>>
>>>
>>> On 8/14/2017 1:36 AM, Sanjoy Das wrote:
>>>> Hi Geoff,
>>>> On Wed, Aug 9, 2017 at 8:58 AM, Geoff Berry <gberry at
codeaurora.org
>>>> <mailto:gberry at codeaurora.org>> wrote:
>>>>> On 8/8/2017 8:38 PM, Sanjoy Das wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Aug 8, 2017 at 12:58 PM, Friedman, Eli 
>>>>>> <efriedma at codeaurora.org <mailto:efriedma at
codeaurora.org>>
>>>>>> wrote:
>>>>>>>
>>>>>>> Oh, I see... yes, we do stupid things involving
mutating NoWrap
>>>>>>> flags
>>>>>>> after
>>>>>>> a SCEV is created.  (grep for setNoWrapFlags in 
>>>>>>> ScalarEvolution.cpp.)
>>>>>>
>>>>>>
>>>>>> That's really a compile time hack -- we defer some
expensive
>>>>>> tricks to
>>>>>> prove nsw/nuw on an add recurrences to when we've
been asked to
>>>>>> sign/zero extend said add recurrence.
>>>>>
>>>>>
>>>>> If that is the case, preserving SCEV analysis where we
didn't
>>>>> before should
>>>>> only
>>>>> result in later passes seeing more nsw/nuw flags, which
should
>>>>> theoretically
>>>>> be
>>>>> beneficial?
>>>> There are downsides to preserving SCEV for "too long"
though -- if you
>>>> simplified a loop in a way that SCEV can compute its trip count
when
>>>> it couldn't before (or it could compute a trip count
before, but now
>>>> it can compute a more precise trip count) then invalidating the
cache
>>>> can be an improvement.  However, in this case judicious use of
>>>> forgetLoop() (as opposed to re-constructing the whole of SCEV)
will
>>>> also do the job.
>>>
>>> In this particular case, neither of the passes that I'm marking
as
>>> newly
>>> preserving SCEV modify any loop structure or any instructions that 
>>> should
>>> impact any of the existing SCEV values.  All they do is add
prefetch
>>> instructions (and some new instructions to compute the addresses to
>>> prefetch) and annotate some load instructions with metadata.
That's why
>>> I was surprised to see changes in generated code from marking them
as
>>> preserving SCEV.
>>>
>>> I've done some performance testing and didn't see any
significant
>>> changes
>>> in performance due to these minor SCEV changes, so I think I'm
going to
>>> post a patch to enable SCEV preservation in these passes and move
on.
>>>
>>>> -- Sanjoy
>>>>>> I would be okay if you wanted to expose those tricks
via a helper in
>>>>>> SCEV that could be called independently of creating
zero or sign
>>>>>> extensions.  That way SCEV clients could trade-off
compile time
>>>>>> for> more
>>>>>> precise information in a granular way.
>>>>>
>>>>>
>>>>> I'm not concerned with getting better SCEV results at
the moment,
>>>>> I just
>>>>> want to
>>>>> make sure that changing these passes to preserve SCEV
analysis is
>>>>> behaving
>>>>> as
>>>>> expected (which it sounds like it is).
>>>>>
>>>>>>
>>>>>> -- Sanjoy
>>>>>>
>>>>>>>
>>>>>>> -Eli
>>>>>>>
>>>>>>> -- 
>>>>>>> Employee of Qualcomm Innovation Center, Inc.
>>>>>>> Qualcomm Innovation Center, Inc. is a member of
Code Aurora
>>>>>>> Forum, a
>>>>>>> Linux
>>>>>>> Foundation Collaborative Project
>>>>>>>
>>>>>
>>>>> -- 
>>>>> Geoff Berry
>>>>> Employee of Qualcomm Datacenter Technologies, Inc.
>>>>>  Qualcomm Datacenter Technologies, Inc. as an affiliate of
Qualcomm
>>>>> Technologies, Inc.  Qualcomm Technologies, Inc. is a member
of the
>>>>> Code
>>>>> Aurora Forum, a Linux Foundation Collaborative Project.
>>>
>>> -- 
>>> Geoff Berry
>>> Employee of Qualcomm Datacenter Technologies, Inc.
>>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
>>> Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the 
>>> Code Aurora Forum, a Linux Foundation Collaborative Project.
>>
>
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory