8241503: C2: Share MacroAssembler between mach nodes during code emission #16484

JohnTortugo · 2023-11-02T22:17:43Z

Description

Please review this PR with a patch to re-use the same C2_MacroAssembler object to emit all instructions in the same compilation unit.

Overall, the change is pretty simple. However, due to the renaming of the variable to access C2_MacroAssembler, from _masm. to masm->, and also some method prototype changes, the patch became quite large.

Help Needed for Testing

I don't have access to all platforms necessary to test this. I hope some other folks can help with testing on S390, RISC-V and PPC.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8241503: C2: Share MacroAssembler between mach nodes during code emission (Enhancement - P3)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to b4d73c98
Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to aa80c801
Amit Kumar (@offamitkumar - Committer) ⚠️ Review applies to b4d73c98
Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to b4d73c98

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16484/head:pull/16484
$ git checkout pull/16484

Update a local copy of the PR:
$ git checkout pull/16484
$ git pull https://git.openjdk.org/jdk.git pull/16484/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16484

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16484.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-02T22:18:42Z

👋 Welcome back cslucas! 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 · 2023-11-02T22:20:30Z

@JohnTortugo The following labels will be automatically applied to this pull request:

graal
hotspot
shenandoah

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

mlbridge · 2023-11-02T22:36:19Z

Webrevs

TheRealMDoerr · 2023-11-02T23:21:23Z

PPC64 runs into assert(masm->inst_mark() == nullptr) failed: should be.
V [libjvm.so+0x1648528] PhaseOutput::fill_buffer(C2_MacroAssembler*, unsigned int*)+0x10c8 (output.cpp:1812)
V [libjvm.so+0x164b35c] PhaseOutput::Output()+0xd5c (output.cpp:362)
V [libjvm.so+0x958f9c] Compile::Code_Gen()+0x4ec (compile.cpp:2989)
V [libjvm.so+0x95e484] Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1a84 (compile.cpp:887)
V [libjvm.so+0x718f58] C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x198 (c2compiler.cpp:119)

JohnTortugo · 2023-11-02T23:36:06Z

@TheRealMDoerr - this is likely because of some missing clear_inst_mark call on my part in the PPC ad, I'll take a look into it.

src/hotspot/cpu/x86/gc/z/z_x86_64.ad

RealFYang · 2023-11-03T04:44:40Z

Hello, I guess you might want to merge latest jdk master and add more changes.
I witnessed some build errors when building the latest jdk master with this patch on linux-riscv64:

/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad: In member function 'virtual void UdivINode::emit(C2_MacroAssembler*, PhaseRegAlloc*) const':
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad:2412:30: error: 'cbuf' was not declared in this scope
 2412 |     C2_MacroAssembler _masm(&cbuf);
      |                              ^~~~
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad: In member function 'virtual void UdivLNode::emit(C2_MacroAssembler*, PhaseRegAlloc*) const':
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad:2427:30: error: 'cbuf' was not declared in this scope
 2427 |     C2_MacroAssembler _masm(&cbuf);
      |                              ^~~~
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad: In member function 'virtual void UmodINode::emit(C2_MacroAssembler*, PhaseRegAlloc*) const':
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad:2442:30: error: 'cbuf' was not declared in this scope
 2442 |     C2_MacroAssembler _masm(&cbuf);
      |                              ^~~~
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad: In member function 'virtual void UmodLNode::emit(C2_MacroAssembler*, PhaseRegAlloc*) const':
/home/fyang/jdk/src/hotspot/cpu/riscv/riscv.ad:2457:30: error: 'cbuf' was not declared in this scope
 2457 |     C2_MacroAssembler _masm(&cbuf);

JohnTortugo · 2023-11-03T16:24:01Z

@RealFYang - Thanks for the note. I'll do that and update the PR.

offamitkumar · 2023-11-08T04:34:03Z

s390x also run into assert failure: assert(masm->inst_mark() == nullptr) failed: should be.

V  [libjvm.so+0xfb0938]  PhaseOutput::fill_buffer(C2_MacroAssembler*, unsigned int*)+0x2370  (output.cpp:1812)
V  [libjvm.so+0xfb21ce]  PhaseOutput::Output()+0xcae  (output.cpp:362)
V  [libjvm.so+0x6a90a8]  Compile::Code_Gen()+0x460  (compile.cpp:2989)
V  [libjvm.so+0x6ad848]  Compile::Compile(ciEnv*, ciMethod*, int, Options, DirectiveSet*)+0x1738  (compile.cpp:887)
V  [libjvm.so+0x4fb932]  C2Compiler::compile_method(ciEnv*, ciMethod*, int, bool, DirectiveSet*)+0x14a  (c2compiler.cpp:119)
V  [libjvm.so+0x6b81a2]  CompileBroker::invoke_compiler_on_method(CompileTask*)+0xd9a  (compileBroker.cpp:2282)
V  [libjvm.so+0x6b8eaa]  CompileBroker::compiler_thread_loop()+0x5a2  (compileBroker.cpp:1943)

JohnTortugo · 2023-11-08T16:48:20Z

Thank you for helping with test @TheRealMDoerr @offamitkumar @RealFYang . I working on an update and I'll push it today or soon after.

openjdk · 2023-11-20T22:37:25Z

@JohnTortugo this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout reuse-macroasm
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Merge remote-tracking branch 'origin/master' into reuse-macroasm

shqking

We should update the copyright year for the following files:

src/hotspot/cpu/aarch64/gc/x/x_aarch64.ad
src/hotspot/cpu/arm/arm_32.ad
src/hotspot/cpu/ppc/gc/x/x_ppc.ad
src/hotspot/cpu/riscv/gc/x/x_riscv.ad
src/hotspot/cpu/x86/c2_intelJccErratum_x86.hpp
src/hotspot/cpu/x86/gc/x/x_x86_64.ad
src/hotspot/cpu/x86/x86_32.ad
src/hotspot/share/opto/c2_CodeStubs.hpp
src/hotspot/share/opto/constantTable.hpp

src/hotspot/cpu/aarch64/aarch64_vector.ad

src/hotspot/cpu/aarch64/aarch64.ad

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp

RealFYang · 2024-03-25T02:07:04Z

@RealFYang - Thanks for the note. I'll do that and update the PR.

Thanks for the update. The RISC-V part looks fine to me. I did release release build and ran tier1-3 tests on linux-riscv64 platform.

offamitkumar

I have done testing on s390 (z15) with: {fastdebug, slowdebug, release} X {tier1} and haven't seen new failure appearing due to this change.

Rest of review leave to @RealLucy :-)

bulasevich · 2024-03-26T14:57:58Z

Hi! Your change has conflict with current jdk master. Please rebase.

RealLucy

s390 changes look good to me.

bulasevich · 2024-03-27T16:12:26Z

FYI. Something goes wrong with the change on ARM32.

#
# A fatal error has been detected by the Java Runtime Environment:
#
#  Internal Error (/ws/workspace/jdk-dev/label/linux-arm/type/b11/jdk/src/hotspot/share/asm/codeBuffer.hpp:163), pid=10782, tid=10796
#  assert(_mark != nullptr) failed: not an offset
#
# JRE version: OpenJDK Runtime Environment (23.0) (fastdebug build 23-commit8fc9097b-adhoc.re.jdk)
# Java VM: OpenJDK Server VM (fastdebug 23-commit8fc9097b-adhoc.re.jdk, mixed mode, g1 gc, linux-arm)
# Problematic frame:
# V  [libjvm.so+0x136ccc]  emit_call_reloc(C2_MacroAssembler*, MachCallNode const*, MachOper*, RelocationHolder const&)+0x2ac
#
# Core dump will be written. Default location: Core dumps may be processed with "/usr/share/apport/apport %p %s %c %d %P %E" (or dumping to /ws/workspace/jdk-dev-jtreg/label/linux-arm/suite/jdk-tier1/type/t11/core.10782)
#
# If you would like to submit a bug report, please visit:
#   https://bugreport.java.com/bugreport/crash.jsp
#

---------------  S U M M A R Y ------------

Command Line:

Host: vm-ubuntu-16v4-aarch64-1, ARM, 4 cores, 7G, Ubuntu 16.04.7 LTS
Time: Wed Mar 27 07:16:41 2024 UTC elapsed time: 0.097440 seconds (0d 0h 0m 0s)

---------------  T H R E A D  ---------------

Current thread (0xb120cd10):  JavaThread "C2 CompilerThread0" daemon [_thread_in_vm, id=10796, stack(0xb1090000,0xb1110000) (512K)]

Stack: [0xb1090000,0xb1110000],  sp=0xb110d200,  free space=500k
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x136ccc]  emit_call_reloc(C2_MacroAssembler*, MachCallNode const*, MachOper*, RelocationHolder const&)+0x2ac  (codeBuffer.hpp:163)
V  [libjvm.so+0x14ca28]  CallRuntimeDirectNode::emit(C2_MacroAssembler*, PhaseRegAlloc*) const+0x80
V  [libjvm.so+0x10ed850]  PhaseOutput::scratch_emit_size(Node const*)+0x37c
V  [libjvm.so+0x10e5f64]  PhaseOutput::shorten_branches(unsigned int*)+0x274
V  [libjvm.so+0x10f6dcc]  PhaseOutput::Output()+0x488
V  [libjvm.so+0x699b54]  Compile::Code_Gen()+0x424
V  [libjvm.so+0x69a87c]  Compile::Compile(ciEnv*, TypeFunc const* (*)(), unsigned char*, char const*, int, bool, bool, DirectiveSet*)+0xb3c
V  [libjvm.so+0x122e73c]  OptoRuntime::generate_stub(ciEnv*, TypeFunc const* (*)(), unsigned char*, char const*, int, bool, bool)+0xb4
V  [libjvm.so+0x122ec68]  OptoRuntime::generate(ciEnv*)+0x50
V  [libjvm.so+0x4994cc]  C2Compiler::init_c2_runtime()+0x104
V  [libjvm.so+0x4996dc]  C2Compiler::initialize()+0x9c
V  [libjvm.so+0x6a371c]  CompileBroker::init_compiler_runtime()+0x35c
V  [libjvm.so+0x6ab90c]  CompileBroker::compiler_thread_loop()+0x178
V  [libjvm.so+0xb30c94]  JavaThread::thread_main_inner()+0x27c
V  [libjvm.so+0x13b3aa0]  Thread::call_run()+0x130
V  [libjvm.so+0x10c98bc]  thread_native_entry(Thread*)+0x13c

JohnTortugo · 2024-03-27T17:02:35Z

Thanks for testing @bulasevich . I'm going to take a look and get back to you asap.

JohnTortugo · 2024-04-04T23:24:16Z

@bulasevich - Is the test that failed one of JDK jtreg tests? Did you include any additional JVM parameter to run the test?

bulasevich · 2024-04-05T02:08:41Z

@bulasevich - Is the test that failed one of JDK jtreg tests? Did you include any additional JVM parameter to run the test?

This happens on VM startup with empty params (no test).

bulasevich · 2024-04-08T06:09:14Z

Do you need help understanding the problem? The crash occurred because you removed the line fprintf(fp, " cbuf.set_insts_mark();\n"); from the generator of AD nodes ::emit() methods. That is why emit_call_reloc finds cbuf.insts->_mark unitialized.

// Call Runtime Instruction
instruct CallRuntimeDirect(method meth) %{
  match(CallRuntime);
  effect(USE meth);
  ins_cost(CALL_COST);
  format %{ "CALL,runtime" %}
  ins_encode( Java_To_Runtime( meth ),
              call_epilog );
  ins_pipe(simple_call);
%}

-->

void CallRuntimeDirectNode::emit(CodeBuffer& cbuf, PhaseRegAlloc* ra_) const {
  cbuf.set_insts_mark();
  // Start at oper_input_base() and count operands
  unsigned idx0 = 1;
  unsigned idx1 = 1;    //
  {
#line 1217 "/home/boris/jdk-bulasevich/src/hotspot/cpu/arm/arm.ad"
    // CALL directly to the runtime
    emit_call_reloc(cbuf, as_MachCall(), opnd_array(1), runtime_call_Relocation::spec());
#line 999999
  }
  {
#line 1213 "/home/boris/jdk-bulasevich/src/hotspot/cpu/arm/arm.ad"
    // nothing
#line 999999
  }
}

JohnTortugo · 2024-04-08T16:28:33Z

Do you need help understanding the problem?

It's hard for me to debug because I don't have direct access to an ARM32. However, I was able to reproduce the problem and I know the reason why it happens (its exactly what you described). I'm working now to find the correct locations to insert the inst_marks() and the clear_inst_marks().

JohnTortugo · 2024-04-09T19:08:08Z

@bulasevich - I just pushed a fix for ARM32. Can you please run your tests again? Thanks!

src/hotspot/cpu/arm/arm.ad

bulasevich · 2024-04-10T21:21:49Z

@bulasevich - I just pushed a fix for ARM32. Can you please run your tests again? Thanks!

Good. Tests are OK now.

JohnTortugo · 2024-04-11T15:34:02Z

Thank you all for reviewing!!

JohnTortugo · 2024-04-11T15:34:06Z

/integrate

openjdk · 2024-04-11T15:34:41Z

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

TheRealMDoerr · 2024-04-11T15:44:18Z

/sponsor

openjdk · 2024-04-11T15:44:51Z

Going to push as commit 31ee510.
Since your change was applied there have been 206 commits pushed to the master branch:

0656f08: 8329469: Generational ZGC: Move the methods forwarding_[index|find|insert] from zRelocate.cpp to ZForwarding
1606187: 8326947: Minimize MakeBase.gmk
2e3682a: 8319678: Several tests from corelibs areas ignore VM flags
63684cd: 8327250: assert(!method->is_old()) failed: Should not be installing old methods
ecc603c: 8201183: sjavac build failures: "Connection attempt failed: Connection refused"
ff5c9a4: 8329528: G1 does not update TAMS correctly when dropping retained regions during Concurrent Start pause
9acce7a: 8329774: Break long lines in jdk/src/jdk.hotspot.agent/doc /transported_core.html
f0cd866: 8329704: Implement framework for proper handling of JDK_LIBS
8817ba4: 8330000: ZGC: ZObjArrayAllocator may unnecessarily clear TypeArrays twice
f778642: 8330024: [AIX] replace my_disclaim64 helper by direct call to disclaim64
... and 196 more: https://git.openjdk.org/jdk/compare/89e0889ab3beb96103ee381d9a54614e49e6d9d7...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-11T15:44:58Z

@TheRealMDoerr @JohnTortugo Pushed as commit 31ee510.

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

Reuse same C2_MacroAssembler object to emit instructions.

06a50bf

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Nov 2, 2023

JohnTortugo marked this pull request as ready for review November 2, 2023 22:30

openjdk bot added the rfr Pull request is ready for review label Nov 2, 2023

dean-long reviewed Nov 3, 2023

View reviewed changes

src/hotspot/cpu/x86/gc/z/z_x86_64.ad Show resolved Hide resolved

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Nov 20, 2023

Catch up with changes on master

b56c98d

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Nov 21, 2023

JohnTortugo added 3 commits November 29, 2023 11:27

Some inst_mark fixes; Catch up with master.

89a6dff

Catch up with master branch.

b239b24

Merge remote-tracking branch 'origin/master' into reuse-macroasm

Fix merge

afe48fe

shqking reviewed Nov 30, 2023

View reviewed changes

src/hotspot/cpu/aarch64/aarch64_vector.ad Show resolved Hide resolved

src/hotspot/cpu/aarch64/aarch64.ad Show resolved Hide resolved

shqking reviewed Dec 1, 2023

View reviewed changes

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp Outdated Show resolved Hide resolved

Fix build, copyright dates, m4 files.

d950806

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 5, 2023

Merge with origin/master

1669279

offamitkumar approved these changes Mar 26, 2024

View reviewed changes

RealLucy approved these changes Mar 26, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into reuse-macroasm

b4d73c9

Fix ARM32 AD file

693c7ef

bulasevich reviewed Apr 10, 2024

View reviewed changes

src/hotspot/cpu/arm/arm.ad Outdated Show resolved Hide resolved

openjdk bot changed the title ~~JDK-8241503: C2: Share MacroAssembler between mach nodes during code emission~~ 8241503: C2: Share MacroAssembler between mach nodes during code emission Apr 10, 2024

Remove unused operands in arm.ad

44e63ee

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 11, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 11, 2024

openjdk bot closed this Apr 11, 2024

openjdk bot 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 Apr 11, 2024

JohnTortugo deleted the reuse-macroasm branch April 11, 2024 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8241503: C2: Share MacroAssembler between mach nodes during code emission #16484

8241503: C2: Share MacroAssembler between mach nodes during code emission #16484

JohnTortugo commented Nov 2, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

mlbridge bot commented Nov 2, 2023 •

edited

Loading

TheRealMDoerr commented Nov 2, 2023

JohnTortugo commented Nov 2, 2023

RealFYang commented Nov 3, 2023 •

edited

Loading

JohnTortugo commented Nov 3, 2023

offamitkumar commented Nov 8, 2023

JohnTortugo commented Nov 8, 2023

openjdk bot commented Nov 20, 2023

shqking left a comment

RealFYang commented Mar 25, 2024

offamitkumar left a comment

bulasevich commented Mar 26, 2024

RealLucy left a comment

bulasevich commented Mar 27, 2024

JohnTortugo commented Mar 27, 2024

JohnTortugo commented Apr 4, 2024

bulasevich commented Apr 5, 2024

bulasevich commented Apr 8, 2024

JohnTortugo commented Apr 8, 2024

JohnTortugo commented Apr 9, 2024

bulasevich commented Apr 10, 2024

JohnTortugo commented Apr 11, 2024

JohnTortugo commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

TheRealMDoerr commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

8241503: C2: Share MacroAssembler between mach nodes during code emission #16484

8241503: C2: Share MacroAssembler between mach nodes during code emission #16484

Conversation

JohnTortugo commented Nov 2, 2023 • edited by openjdk bot Loading

Description

Help Needed for Testing

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 2, 2023

openjdk bot commented Nov 2, 2023

mlbridge bot commented Nov 2, 2023 • edited Loading

Webrevs

TheRealMDoerr commented Nov 2, 2023

JohnTortugo commented Nov 2, 2023

RealFYang commented Nov 3, 2023 • edited Loading

JohnTortugo commented Nov 3, 2023

offamitkumar commented Nov 8, 2023

JohnTortugo commented Nov 8, 2023

openjdk bot commented Nov 20, 2023

shqking left a comment

Choose a reason for hiding this comment

RealFYang commented Mar 25, 2024

offamitkumar left a comment

Choose a reason for hiding this comment

bulasevich commented Mar 26, 2024

RealLucy left a comment

Choose a reason for hiding this comment

bulasevich commented Mar 27, 2024

JohnTortugo commented Mar 27, 2024

JohnTortugo commented Apr 4, 2024

bulasevich commented Apr 5, 2024

bulasevich commented Apr 8, 2024

JohnTortugo commented Apr 8, 2024

JohnTortugo commented Apr 9, 2024

bulasevich commented Apr 10, 2024

JohnTortugo commented Apr 11, 2024

JohnTortugo commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

TheRealMDoerr commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

openjdk bot commented Apr 11, 2024

JohnTortugo commented Nov 2, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 2, 2023 •

edited

Loading

RealFYang commented Nov 3, 2023 •

edited

Loading