RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Vladimir Kozlov-2
When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)

I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.

Tested hs-tier1-4

-------------

Commit messages:
 - 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Changes: https://git.openjdk.java.net/jdk/pull/3365/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3365&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264063
  Stats: 47 lines in 6 files changed: 10 ins; 36 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3365.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3365/head:pull/3365

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Pengfei Li
On Wed, 7 Apr 2021 02:29:20 GMT, Vladimir Kozlov <[hidden email]> wrote:

> When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)
>
> I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.
>
> Tested hs-tier1-4

Should we also remove below part of code in `loopTransform.cpp` in this patch?

3704         if (n->is_CountedLoop() && n->as_CountedLoop()->is_strip_mined()) {
3705           // In strip-mined counted loops, the CountedLoopNode may be
3706           // used by the address polling node of the outer safepoint.
3707           // Skip this use because it's safe.
3708           Node* sfpt = n->as_CountedLoop()->outer_safepoint();
3709           Node* polladr = sfpt->in(TypeFunc::Parms+0);
3710           if (use == polladr) {
3711             continue;
3712           }
3713         }

-------------

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Vladimir Kozlov-2
On Wed, 7 Apr 2021 03:48:15 GMT, Pengfei Li <[hidden email]> wrote:

> Should we also remove below part of code in `loopTransform.cpp` in this patch?
>
> ```
> 3704         if (n->is_CountedLoop() && n->as_CountedLoop()->is_strip_mined()) {
> 3705           // In strip-mined counted loops, the CountedLoopNode may be
> 3706           // used by the address polling node of the outer safepoint.
> 3707           // Skip this use because it's safe.
> 3708           Node* sfpt = n->as_CountedLoop()->outer_safepoint();
> 3709           Node* polladr = sfpt->in(TypeFunc::Parms+0);
> 3710           if (use == polladr) {
> 3711             continue;
> 3712           }
> 3713         }
> ```

I thought that it does not harm to have this code but on other hand it will be not executed anymore.
I will removed it and I will run ArrayFill.java to make sure optimization works with strip mined loops.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop. [v2]

Vladimir Kozlov-2
In reply to this post by Vladimir Kozlov-2
> When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)
>
> I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.
>
> Tested hs-tier1-4

Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:

  Removed unneeded anymore code in match_fill_loop()

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3365/files
  - new: https://git.openjdk.java.net/jdk/pull/3365/files/1f645337..58a4ce1d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3365&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3365&range=00-01

  Stats: 10 lines in 1 file changed: 0 ins; 10 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3365.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3365/head:pull/3365

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Vladimir Kozlov-2
In reply to this post by Vladimir Kozlov-2
On Wed, 7 Apr 2021 04:53:44 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Should we also remove below part of code in `loopTransform.cpp` in this patch?
>>
>> 3704         if (n->is_CountedLoop() && n->as_CountedLoop()->is_strip_mined()) {
>> 3705           // In strip-mined counted loops, the CountedLoopNode may be
>> 3706           // used by the address polling node of the outer safepoint.
>> 3707           // Skip this use because it's safe.
>> 3708           Node* sfpt = n->as_CountedLoop()->outer_safepoint();
>> 3709           Node* polladr = sfpt->in(TypeFunc::Parms+0);
>> 3710           if (use == polladr) {
>> 3711             continue;
>> 3712           }
>> 3713         }
>
>> Should we also remove below part of code in `loopTransform.cpp` in this patch?
>>
>> ```
>> 3704         if (n->is_CountedLoop() && n->as_CountedLoop()->is_strip_mined()) {
>> 3705           // In strip-mined counted loops, the CountedLoopNode may be
>> 3706           // used by the address polling node of the outer safepoint.
>> 3707           // Skip this use because it's safe.
>> 3708           Node* sfpt = n->as_CountedLoop()->outer_safepoint();
>> 3709           Node* polladr = sfpt->in(TypeFunc::Parms+0);
>> 3710           if (use == polladr) {
>> 3711             continue;
>> 3712           }
>> 3713         }
>> ```
>
> I thought that it does not harm to have this code but on other hand it will be not executed anymore.
> I will removed it and I will run ArrayFill.java to make sure optimization works with strip mined loops.

I removed pointed code and verified that arrayfill optimization still works with strip mined loops.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop. [v2]

Roland Westrelin-4
In reply to this post by Vladimir Kozlov-2
On Wed, 7 Apr 2021 16:05:01 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)
>>
>> I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.
>>
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Removed unneeded anymore code in match_fill_loop()

Looks good to me.

-------------

Marked as reviewed by roland (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop. [v2]

Vladimir Ivanov-2
In reply to this post by Vladimir Kozlov-2
On Wed, 7 Apr 2021 16:05:01 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)
>>
>> I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.
>>
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>
>   Removed unneeded anymore code in match_fill_loop()

Looks good.

-------------

Marked as reviewed by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop. [v2]

Vladimir Kozlov-2
On Thu, 8 Apr 2021 12:38:36 GMT, Vladimir Ivanov <[hidden email]> wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Removed unneeded anymore code in match_fill_loop()
>
> Looks good.

Thank you Vladimir and Roland.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3365
Reply | Threaded
Open this post in threaded view
|

Integrated: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Vladimir Kozlov-2
In reply to this post by Vladimir Kozlov-2
On Wed, 7 Apr 2021 02:29:20 GMT, Vladimir Kozlov <[hidden email]> wrote:

> When loop is "strip mined" polling address load ([parse1.cpp#L2280](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/parse1.cpp#L2280)) should be cloned together with safepoint node and pinned outside inner loop. Otherwise we have issues like [8263352](https://bugs.openjdk.java.net/browse/JDK-8263352)
>
> I also remove leftover (unused needs_polling_address_input() method) from  [8220051](https://bugs.openjdk.java.net/browse/JDK-8220051) changes.
>
> Tested hs-tier1-4

This pull request has now been integrated.

Changeset: 81d35e43
Author:    Vladimir Kozlov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/81d35e43
Stats:     57 lines in 7 files changed: 10 ins; 46 del; 1 mod

8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.

Reviewed-by: roland, vlivanov

-------------

PR: https://git.openjdk.java.net/jdk/pull/3365