JDK-8277496 Remove duplication in c1 Block successor lists #6614

LudwikJaniuk · 2021-11-30T14:14:36Z

Remove BlockBegin::_successors, leaving BlockEnd::_sux as the SSOT for the successors of a block. Prior to this PR, these two lists were both tracking the same list of successors of the same block. This necessitated a lot of syncing and verification code.

With this PR, as long as a block has its end pointer assigned, its successors can always be reached by querying the BlockEnd. BlockEnd::_sux becomes the single place where the list of successors is maintained. When modified, the successor list no longer needs to be synchronized in two places, reducing complexity and confusion. Asserts on the two lists corresponding no longer need to be made.

While being created in GraphBuilder, BlockBegins don't have a BlockEnd assigned yet. To temporarily track block successors in this small interval, add a lookup structure BlockListBuilder::_bci2block_successors.

This PR affects debug printing code. If the end pointer of a BlockBegin is NULL for some reason, then the successor list can no longer be printed (for obious reasons).

This PR introduces an additional check to IR::verify to check that BlockBegin::_end is not set to null.

This PR also performs some minor refactoring, polishing, inlining, and removing of dead code around the affected areas.

The commit history has been polished to attempt to guide the reader through the changes.

hs-tier1 and hs-tier2 tests pass.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277496: Remove duplication in c1 Block successor lists

Reviewers

Nils Eliasson (@neliasso - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6614/head:pull/6614
$ git checkout pull/6614

Update a local copy of the PR:
$ git checkout pull/6614
$ git pull https://git.openjdk.java.net/jdk pull/6614/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6614

View PR using the GUI difftool:
$ git pr show -t 6614

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6614.diff

create separate versions for two cases _end == NULL, _end != NULL. This makes it explicit that the former case is only ever used in GraphBuilder (and in the printer). GraphBuilder accessor methods for _successors: In preparation for creating a temporary list of lists in graphbuilder

This makes it clear how weird this is. The nonnull case doesnt even set begin!

bridgekeeper · 2021-11-30T14:14:43Z

👋 Welcome back LudwikJaniuk! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-11-30T14:16:28Z

@LudwikJaniuk The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-11-30T14:20:33Z

Webrevs

neliasso

A very nice clean up!

neliasso · 2021-11-30T15:52:32Z

src/hotspot/share/c1/c1_GraphBuilder.cpp

  if (PrintCompilation && Verbose) tty->print_cr("Created %d Instructions", _instruction_count);
 #endif
+
+  // JANIUK: If we iterate all the blocks in _blocks, some of them have end NULL.


Left over comment?

Indeed! Removed it

neliasso · 2021-12-01T12:10:01Z

src/hotspot/share/c1/c1_LIR.cpp

    }
  }

+  if (end != NULL)


Missing braces on if statement

Corrected it

neliasso

Looks good! Thanks for fixing!

openjdk · 2021-12-01T13:44:02Z

@LudwikJaniuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8277496: Remove duplication in c1 Block successor lists

Reviewed-by: neliasso, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 96 new commits pushed to the master branch:

24e16ac: 8277617: Adjust AVX3Threshold for copy/fill stubs
2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
e1cde19: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
a729a70: 8225181: KeyStore should have a getAttributes method
38f525e: 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh()
780b8b1: 8278179: Enable all doclint warnings for build of java.naming
678ac58: 8278240: ProblemList containers/docker/TestJcmd.java on linux-aarch64
01cb2b9: 8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint
660f21a: 8278119: ProblemList few headful test failing in macosx12-aarch64 system
2e30fa9: 8278171: [vectorapi] Mask incorrectly computed for zero extending cast
... and 86 more: https://git.openjdk.java.net/jdk/compare/a5f2a58ba4ac25f4bd66f1f1f4c036a4f0024229...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@neliasso, @vnkozlov) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

LudwikJaniuk · 2021-12-02T14:24:30Z

/integrate

openjdk · 2021-12-02T14:25:33Z

@LudwikJaniuk
Your change (at version dbe1301) is now ready to be sponsored by a Committer.

vnkozlov

@LudwikJaniuk Please, rerun your testing. Running only -b linux-x64 is not enough - you should not specify this option at all. Testing should be done on all platforms. Also linux-x64 is product build which does not include asserts. So we may not catch possible issues.

vnkozlov

Good.
Testing results looks good too.

neliasso · 2021-12-06T08:57:26Z

/sponsor

openjdk · 2021-12-06T08:58:50Z

Going to push as commit 8d190dd.
Since your change was applied there have been 104 commits pushed to the master branch:

194cdf4: 8277864: Compilation error thrown while doing a boxing conversion on selector expression
f39fe5b: 8154011: Make TraceDeoptimization a diagnostic flag
f180a45: 8278016: Add compiler tests to tier{2,3}
104aa1f: 8268575: Annotations not visible on model elements before they are generated
839b606: 8278143: Remove unused "argc" from ConstantPool::copy_bootstrap_arguments_at_impl
267c024: 8265150: AsyncGetCallTrace crashes on ResourceMark
9642629: 8276779: (ch) InputStream returned by Channels.newInputStream should have fast path for SelectableChannels
02ee337: 8278175: Enable all doclint warnings for build of java.desktop
24e16ac: 8277617: Adjust AVX3Threshold for copy/fill stubs
2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
... and 94 more: https://git.openjdk.java.net/jdk/compare/a5f2a58ba4ac25f4bd66f1f1f4c036a4f0024229...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-06T08:59:06Z

@neliasso @LudwikJaniuk Pushed as commit 8d190dd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

LudwikJaniuk added 28 commits November 30, 2021 03:34

inline remove_successor

22b4539

Clarify where end can be null

14c7709

split set_begin

8818dcd

This makes it clear how weird this is. The nonnull case doesnt even set begin!

make a point that we can delete

b2803bd

rm clear_begin(), was doing nothing

189a03d

inline clear_end

14dbf2f

remove set_sux_from_begin, dead

dfba0fd

Inline disconnect_from_graph

fbafd16

Add comment about my understanding

3793dfc

add explicit local version add_successor

0d58c3a

Create local successor structure in graphbuilder

c09d1ce

Use local sux graphbuilder, obsolete _successors

9174556

remove comment about my understanding

7db1dda

nicer names and placement in graphbuilder

1062a74

Remove null and match checks using successors

6d41a9a

remove _successors field and accessor

968fa50

make BlockEnd::sux() protected

2316e7d

improve set_end

0a4e7a5

remove dead code

0d57b50

simplify blockmerger

870a430

IR verify that ends aren't null

5be57ec

Remove extraneous assertions in build_hir

34d7315

CFGPrinter don't print sux when end is NULL

405481f

comment on end == null in BlockListBuilder()

24017bc

add is_successor

aef8c60

rm duplicated printing

90ca1f6

extract disconnect_from_graph

284cc91

openjdk bot added the rfr Pull request is ready for review label Nov 30, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 30, 2021

neliasso reviewed Nov 30, 2021

View reviewed changes

remove stray comment

ec61358

neliasso reviewed Dec 1, 2021

View reviewed changes

changed if statement style

dbe1301

neliasso approved these changes Dec 1, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 2, 2021

vnkozlov reviewed Dec 3, 2021

View reviewed changes

vnkozlov approved these changes Dec 4, 2021

View reviewed changes

openjdk bot closed this Dec 6, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 6, 2021

JDK-8277496 Remove duplication in c1 Block successor lists #6614

JDK-8277496 Remove duplication in c1 Block successor lists #6614

Conversation

LudwikJaniuk commented Nov 30, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 30, 2021

Uh oh!

openjdk bot commented Nov 30, 2021

Uh oh!

mlbridge bot commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

neliasso left a comment

Choose a reason for hiding this comment

Uh oh!

neliasso Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

LudwikJaniuk Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

neliasso Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

LudwikJaniuk Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

neliasso left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LudwikJaniuk commented Dec 2, 2021

Uh oh!

openjdk bot commented Dec 2, 2021

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

neliasso commented Dec 6, 2021

Uh oh!

openjdk bot commented Dec 6, 2021

Uh oh!

openjdk bot commented Dec 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

LudwikJaniuk commented Nov 30, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 30, 2021 •

edited

Loading

openjdk bot commented Dec 1, 2021 •

edited

Loading