Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8241502: C2: Migrate x86_64.ad to MacroAssembler #2420

Closed
wants to merge 15 commits into from
Closed

8241502: C2: Migrate x86_64.ad to MacroAssembler #2420

wants to merge 15 commits into from

Conversation

@JohnTortugo
Copy link
Contributor

@JohnTortugo JohnTortugo commented Feb 5, 2021

Relates to: https://bugs.openjdk.java.net/browse/JDK-8241502
Tested on: Linux tier1, 2 and 3

Can you please take a look whether these changes are going in the direction expected or not? If it is, I'll continue working on the JDK-8241502 but I'd like to split it in a few PRs since it's a lot of changes.


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2420

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 5, 2021

👋 Welcome back JohnTortugo! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 5, 2021

@JohnTortugo 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 5, 2021

⚠️ @JohnTortugo This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Loading

@JohnTortugo JohnTortugo changed the title Migrate x86_64.ad to MacroAssembler 8241502: Migrate x86_64.ad to MacroAssembler Feb 5, 2021
@openjdk openjdk bot added the rfr label Feb 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 5, 2021

Loading

@iwanowww
Copy link

@iwanowww iwanowww commented Feb 5, 2021

Can you please take a look whether these changes are going in the direction expected or not?

Yes, the patch is perfectly aligned with what JDK-8241502 proposes. Thanks for taking care of it.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Feb 5, 2021

I agree. We wanted to do that for long time.

Loading

@iklam
Copy link
Member

@iklam iklam commented Feb 5, 2021

I am curious if the x86_64.o file changes in any significant way (speed of size).

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Feb 9, 2021

I wish there was a way for the old and new versions to co-exist at the same time, so we could generate the code the old way and and the new way, then compare, for automatic verification of the MacroAssember version.

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented Feb 11, 2021

Thank you all for the feedback!

@iklam - I'll check that and let you know once I make more conversions.

@dean-long - That would be great. I'm all ears for the best way to test this!

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Feb 17, 2021

Here's one way to test both versions, using loadRange as an example. It's not exactly pretty, but it seems to work.
loadRange.txt

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented Feb 17, 2021

@dean-long - I recently hacked something pretty similar - basically, I added a flag to the CodeSection class to make it print emitted bytes whenever the flag was set, then I created two encoding classes in the AD file to toggle the print flag. That way I was able to visually compare the output of the two versions of the code. Your approach with the CRC is much better. Thanks a lot!

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented Mar 6, 2021

Hi again, here is a quick update on the progress of this and an ask for advice.

I finished doing all conversions. I'm using an automated approach to validate the conversions: for each instruction that I converted I created an enc_class (just for debugging purposes) with pretty much the code that @dean-long provided above. However, I'm hitting a limitation on test coverage. I performed 248 conversions, but until now I was able to test only 203! I've tested tier1, tier2, and tier3 with different GCs, different VM options, on Linux and macOS. In other words, I've tried several things that I could think of but I couldn't make C2 use some instructions.

I created a Gist with the list of instructions I couldn't test so far. Can you suggest something I can do to make C2 use those additional instructions?

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Mar 15, 2021

Was -Xcomp one of the flags you tried? If so, then you may need to write new tests for those instructions without test coverage. Or you could do the 203 first and file a new RFE for the remaining.

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented Mar 17, 2021

Hi @dean-long, thanks for the suggestions. Yes, "-Xcomp" is among the flags that I tried. In the end, what helped the most was commenting out some optimizations that simplify the IR graph - i.e., I commented out a bunch of ::Ideal methods. I also tried commenting out some instructions in the .ad file and also changing ins_costs. These things helped quite a lot and I was able to get 26 more conversions validated; now the total of conversions + validations is 239. The conversions that I couldn't validate I didn't include in the PR; I'll follow your suggestion and create an RFE once the current PR is merged.

So, please consider this PR ready for review. I've tested these changes on Linux, Windows, and macOS with fastdebug and release binaries running jdk_tier1/2/3 and hotspot_all. Tests were also performed with different GCs: ZGC, Shenandoah, and G1.

I didn't remove the, now unused, enc_classes and related code; I think it would be better to keep them for a few more days to make easier any eventual debugging. I'll be happy to remove them if you think that's a better approach.

Loading

@dean-long
Copy link
Member

@dean-long dean-long commented Mar 25, 2021

Hi @JohnTortugo, good job on the instruction validation. I'm OK with leaving the unused enc_classes for now, but let's see what the other reviewers think. One option would be to move them to a separate .ad file.

I guess you counted an instruction validated if it was generated at least once (so at least 1 possible encoding)? On RISC architectures that would probably be enough, but I know on x86/x64, register choice can change the instruction encoding and length, thanks to prefixes, etc. Unfortunately, I'm out of clever ideas on how to verify all possible encodings of the new AD instructions.

Loading

Copy link

@iwanowww iwanowww left a comment

Looks good.

Loading

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 25, 2021

I didn't remove the, now unused, enc_classes and related code; I think it would be better to keep them for a few more days to make easier any eventual debugging. I'll be happy to remove them if you think that's a better approach.

Can you elaborate, please, what you mean here? I don't see "enc_classes and related code" in the latest patch.

Loading

@iwanowww
Copy link

@iwanowww iwanowww commented Mar 25, 2021

I guess you counted an instruction validated if it was generated at least once (so at least 1 possible encoding)? On RISC architectures that would probably be enough, but I know on x86/x64, register choice can change the instruction encoding and length, thanks to prefixes, etc.

One possibility is to write a native test (using gtest) and exercise some encoding methods by enumerating all the possible input value locations. Probably, too much for a regularly executed test (there's a lot of stuff missing right now to support such scenario), but something which can be quickly hacked for a one-time run.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 22, 2021

I got a lot of failures on Windows only. Almost all test/hotspot/jtreg/serviceability/sa/ tests failed and others.
Examples:

serviceability/sa/TestSysProps.java

#  Internal Error (t:\\workspace\\open\\src\\hotspot\\share\\runtime\\stackValue.cpp:139), pid=7572, tid=26052
#  assert(oopDesc::is_oop_or_null(val, false)) failed: bad oop found

compiler/c2/Test6800154.java
java.lang.InternalError: 9223372036854775807 / -9223372036854775808 failed: 65547 != 0
	at compiler.c2.Test6800154.run(Test6800154.java:111)
	at compiler.c2.Test6800154.main(Test6800154.java:97)

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented Apr 29, 2021

@vnkozlov - thank you so much for running the tests! The cause of the problems you reported were the last changes I made to the div/mod instructions. I fixed the code and ran all tests again on Linux, macOS, and Windows and they are looking good (jdk tier1, 2, 3, and hotspot_all.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 30, 2021

I started new testing.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 30, 2021

Preliminary result shows compiler/c2/cr6340864/TestLongVect.java test failure on linux and windows -x64 when run with not G1 gc (Serial, Parallel, ZGC) (the line could be different again due to use of latest source base):

#  Internal Error (/workspace/open/src/hotspot/cpu/x86/assembler_x86.cpp:10753), pid=9483, tid=9495
#  assert(isByte(imm8)) failed: not a byte
#
# JRE version: Java(TM) SE Runtime Environment (17.0) (fastdebug build 17-internal+0-LTS-2021-04-30-0438565.jdkgit)
# Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 17-internal+0-LTS-2021-04-30-0438565.jdkgit, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, parallel gc, linux-amd64)
# Problematic frame:
# V  [libjvm.so+0x6471b4]  Assembler::sarq(RegisterImpl*, int)+0x164

V  [libjvm.so+0x6471b4]  Assembler::sarq(RegisterImpl*, int)+0x164
V  [libjvm.so+0x34d566]  sarL_rReg_immNode::emit(CodeBuffer&, PhaseRegAlloc*) const+0xb6
V  [libjvm.so+0x1588551]  PhaseOutput::scratch_emit_size(Node const*)+0x421
V  [libjvm.so+0x157f3f8]  PhaseOutput::shorten_branches(unsigned int*)+0x2b8
V  [libjvm.so+0x159165a]  PhaseOutput::Output()+0xcca
V  [libjvm.so+0xa0636b]  Compile::Code_Gen()+0x43b

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Apr 30, 2021

Got additional result and it failed with G1 too.

Loading

JohnTortugo added 2 commits May 3, 2021
Updating branch with latest changes in upstream/master
@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented May 4, 2021

@JohnTortugo did you fix the last issue? Let me know when I should test it.

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented May 4, 2021

@vnkozlov - I've fixed the code, I'm finishing running some more tests. I'll push the new changes later today. Thanks!

Loading

@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented May 8, 2021

Hi @vnkozlov - I just pushed a patch that will handle the assert triggering problem. Thanks again for helping!

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented May 9, 2021

I started testing for latest changes (10).

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

tier1-5 passed clean

Loading

@openjdk openjdk bot added ready and removed ready labels May 10, 2021
@JohnTortugo
Copy link
Contributor Author

@JohnTortugo JohnTortugo commented May 10, 2021

Thank you @vnkozlov !

/integrate

Loading

@openjdk openjdk bot added the sponsor label May 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@JohnTortugo
Your change (at version 7599ed7) is now ready to be sponsored by a Committer.

Loading

@openjdk openjdk bot added ready and removed ready labels May 10, 2021
@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented May 10, 2021

/sponsor

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

@vnkozlov @JohnTortugo Since your change was applied there have been 98 commits pushed to the master branch:

  • c8b7447: 8266603: jpackage: Add missing copyright file in Java runtime .deb installers
  • c494efc: 8266774: System property values for stdout/err on Windows UTF-8
  • 25d99e5: 8266330: itableMethodEntry::initialize() asserts with archived old classes
  • 5d761fc: 8266796: Clean up the unnecessary code in the method UnsharedNameTable#fromUtf
  • e41fd73: 8266252: Streamline AbstractInterpreter::method_kind
  • b823b3e: 8266797: Fix for 8266610 breaks the build on macos
  • 53db2a0: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline
  • 1603ca2: 8241248: NullPointerException in sun.security.ssl.HKDF.extract(HKDF.java:93)
  • 0f925d1: 8266015: Implement AdapterHandlerLibrary lookup fast-path for common adapters
  • 69b96f9: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux.
  • ... and 88 more: https://git.openjdk.java.net/jdk/compare/45760d4baf5da7537e1bae70796e869309d4aeff...master

Your commit was automatically rebased without conflicts.

Pushed as commit de78431.

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

Loading

@JohnTortugo JohnTortugo deleted the jdk-8241502 branch May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants