Skip to content

Commit

Permalink
8275847: Scheduling fails with "too many D-U pinch points" on small m…
Browse files Browse the repository at this point in the history
…ethod

Reviewed-by: thartmann, kvn
  • Loading branch information
nick-arm committed Nov 8, 2021
1 parent 44047f8 commit 3934fe5
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 15 deletions.
8 changes: 7 additions & 1 deletion src/hotspot/cpu/x86/vmreg_x86.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,13 @@ inline bool is_concrete() {
#ifndef AMD64
if (is_Register()) return true;
#endif // AMD64
return is_even(value());
// Do not use is_XMMRegister() here as it depends on the UseAVX setting.
if (value() >= ConcreteRegisterImpl::max_fpr && value() < ConcreteRegisterImpl::max_xmm) {
int base = value() - ConcreteRegisterImpl::max_fpr;
return base % XMMRegisterImpl::max_slots_per_register == 0;
} else {
return is_even(value()); // General, float, and K registers are all two slots wide
}
}

#endif // CPU_X86_VMREG_X86_HPP
18 changes: 4 additions & 14 deletions src/hotspot/share/opto/buildOopMap.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2002, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -231,10 +231,6 @@ OopMap *OopFlow::build_oop_map( Node *n, int max_reg, PhaseRegAlloc *regalloc, i

VMReg r = OptoReg::as_VMReg(OptoReg::Name(reg), framesize, max_inarg_slot);

if (false && r->is_reg() && !r->is_concrete()) {
continue;
}

// See if dead (no reaching def).
Node *def = _defs[reg]; // Get reaching def
assert( def, "since live better have reaching def" );
Expand Down Expand Up @@ -312,14 +308,10 @@ OopMap *OopFlow::build_oop_map( Node *n, int max_reg, PhaseRegAlloc *regalloc, i
set_live_bit(live,breg);
// Already missed our turn?
if( breg < reg ) {
if (b->is_stack() || b->is_concrete() || true ) {
omap->set_oop( b);
}
omap->set_oop(b);
}
}
if (b->is_stack() || b->is_concrete() || true ) {
omap->set_derived_oop( r, b);
}
omap->set_derived_oop(r, b);
}

} else if( t->isa_narrowoop() ) {
Expand Down Expand Up @@ -347,9 +339,7 @@ OopMap *OopFlow::build_oop_map( Node *n, int max_reg, PhaseRegAlloc *regalloc, i
assert( dup_check[_callees[reg]]==0, "trying to callee save same reg twice" );
debug_only( dup_check[_callees[reg]]=1; )
VMReg callee = OptoReg::as_VMReg(OptoReg::Name(_callees[reg]));
if ( callee->is_concrete() || true ) {
omap->set_callee_saved( r, callee);
}
omap->set_callee_saved(r, callee);

} else {
// Other - some reaching non-oop value
Expand Down
10 changes: 10 additions & 0 deletions src/hotspot/share/opto/output.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2915,6 +2915,16 @@ void Scheduling::anti_do_def( Block *b, Node *def, OptoReg::Name def_reg, int is
if( !OptoReg::is_valid(def_reg) ) // Ignore stores & control flow
return;

if (OptoReg::is_reg(def_reg)) {
VMReg vmreg = OptoReg::as_VMReg(def_reg);
if (vmreg->is_reg() && !vmreg->is_concrete() && !vmreg->prev()->is_concrete()) {
// This is one of the high slots of a vector register.
// ScheduleAndBundle already checked there are no live wide
// vectors in this method so it can be safely ignored.
return;
}
}

Node *pinch = _reg_node[def_reg]; // Get pinch point
if ((pinch == NULL) || _cfg->get_block_for_node(pinch) != b || // No pinch-point yet?
is_def ) { // Check for a true def (not a kill)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2021, Arm Limited. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

package compiler.c2.irTests;

import jdk.test.lib.Asserts;
import compiler.lib.ir_framework.*;

/*
* @test
* @bug 8275847
* @requires vm.compiler2.enabled
* @summary Test that small method with runtime calls can be scheduled.
* @library /test/lib /
* @run driver compiler.c2.irTests.TestScheduleSmallMethod
*/
public class TestScheduleSmallMethod {

public static void main(String[] args) {
TestFramework framework = new TestFramework();
Scenario schedulerOn = new Scenario(0, "-XX:+OptoScheduling");
Scenario schedulerOff = new Scenario(1, "-XX:-OptoScheduling");
framework.addScenarios(schedulerOn, schedulerOff).start();
}

@Test
public double testSmallMethodTwoRuntimeCalls(double value) {
// The two intrinsified Math calls below caused the scheduler to
// bail out with "too many D-U pinch points". See bug 8275847.
return Math.log(Math.sin(value));
}

@Run(test = "testSmallMethodTwoRuntimeCalls")
public void checkTestSmallMethodTwoRuntimeCalls() throws Throwable {
Asserts.assertLessThan(testSmallMethodTwoRuntimeCalls(Math.PI/2), 0.00001);
}
}

3 comments on commit 3934fe5

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GoeLin
Copy link
Member

@GoeLin GoeLin commented on 3934fe5 Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/backport jdk17u-dev

@openjdk
Copy link

@openjdk openjdk bot commented on 3934fe5 Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GoeLin the backport was successfully created on the branch GoeLin-backport-3934fe54 in my personal fork of openjdk/jdk17u-dev. To create a pull request with this backport targeting openjdk/jdk17u-dev:master, just click the following link:

➡️ Create pull request

The title of the pull request is automatically filled in correctly and below you find a suggestion for the pull request body:

Hi all,

This pull request contains a backport of commit 3934fe54 from the openjdk/jdk repository.

The commit being backported was authored by Nick Gasson on 8 Nov 2021 and was reviewed by Tobias Hartmann and Vladimir Kozlov.

Thanks!

If you need to update the source branch of the pull then run the following commands in a local clone of your personal fork of openjdk/jdk17u-dev:

$ git fetch https://github.com/openjdk-bots/jdk17u-dev GoeLin-backport-3934fe54:GoeLin-backport-3934fe54
$ git checkout GoeLin-backport-3934fe54
# make changes
$ git add paths/to/changed/files
$ git commit --message 'Describe additional changes made'
$ git push https://github.com/openjdk-bots/jdk17u-dev GoeLin-backport-3934fe54

Please sign in to comment.