Initial support for inline types by MichaelHaas99 · Pull Request #1 · MichaelHaas99/valhalla

MichaelHaas99 · 2024-09-18T14:00:28Z

No description provided.

davleopo · 2024-10-28T12:59:49Z

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp

-      klass = *((Klass**) (intptr_t) (base_address + offset));
+      intptr_t temp = (intptr_t) *((Klass**) (intptr_t) (base_address + offset));
+      // clear the bits used for acmp data profiling
+      clear_bits(temp, 0b011);


use a constant for the bitmask please with proper doc

Reviewed-by: jdv, prr

Reviewed-by: aivanov

dougxc

I left a first round of comments and think they should be addressed before more reviewing. A more general comment is that new JVMCI tests should be added for as much as possible of the new JVMCI code.

dougxc · 2025-03-03T19:48:16Z

src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp


      call = nativeCall_at(_instructions->start() + pc_offset);
      call->set_destination(SharedRuntime::get_resolve_virtual_call_stub());
+      if(method()->has_scalarized_args()) {


Space between if and (. Same everywhere else.

dougxc · 2025-03-03T19:58:13Z

src/hotspot/cpu/x86/jvmciCodeInstaller_x86.cpp

+      if(method()->has_scalarized_args()) {
      _instructions->relocate(call->instruction_address(),
-                                             virtual_call_Relocation::spec(_invoke_mark_pc),
+                                             virtual_call_Relocation::spec(_invoke_mark_pc, _oop_recorder->find_index(method())),


Please find all the places in the aarch64 code that will need similar changes and try stub it out or at least put a TBD comment.

dougxc · 2025-03-03T20:06:38Z

src/hotspot/share/code/nmethod.cpp

      // If inline types are passed as fields, use the extended signature
      // which contains the types of all (oop) fields of the inline type.
-      if (is_compiled_by_c2() && callee->has_scalarized_args()) {
+      if ((is_compiled_by_c2() || is_compiled_by_jvmci()) && callee->has_scalarized_args()) {


Maybe it's worth adding a nmethod::can_scalarize_args abstraction?

dougxc · 2025-03-03T20:13:39Z

src/hotspot/share/code/nmethod.hpp

      return method()->c1_needs_stack_repair();
-    } else if (is_compiled_by_c2()) {
+    } else if (is_compiled_by_c2() || is_compiled_by_jvmci()) {
      return method()->c2_needs_stack_repair();


rename c2_needs_stack_repair to c2_or_jvmci_needs_stack_repair. This will hopefully prompt a follow-on discussion with the Valhalla team about a better name.

Probably change that if we are sure that Graal should also rely on the calling convention.

dougxc · 2025-03-03T20:15:00Z

src/hotspot/share/interpreter/linkResolver.hpp

-                             Bytecodes::Code byte, bool check_null_and_abstract, TRAPS);
+                             Bytecodes::Code byte, bool check_null_or_abstract, TRAPS);
+
+  static void resolve_invoke_jvmci(CallInfo& result, Handle recv, const constantPoolHandle& pool, int index, Bytecodes::Code byte, bool check_null_or_abstract, TRAPS);


Dead code? Doesn't seem to be implemented.

Yes forgot to delete it.

dougxc · 2025-03-03T21:01:36Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java

    @Override
    ResolvedJavaType getArrayClass();

+    default ResolvedJavaType getFlatArrayClass() {


Every method in jdk.vm.ci.meta needs javadoc.

dougxc · 2025-03-03T21:05:04Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaType.java

    }
+
+    /**
+     * Checks whether this type has an identity.


What does "identity" mean here? Where ever possible, cross reference core reflection classes and methods (e.g. Class.isIdentity()) that explain these concepts. The more JVMCI can lean on core Java documentation, the easier it will be to understand (and the better the chance of it staying consistent).

dougxc · 2025-03-03T21:06:20Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/SingleTypeEntry.java

@@ -0,0 +1,19 @@
+package jdk.vm.ci.hotspot;
+
+public interface SingleTypeEntry{


javadoc for the class and all methods please

dougxc · 2025-03-03T21:07:27Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/RegisterConfig.java

+    /**
+     * Gets the registers to be used for returning multiple values e.g. used for a scalarized inline object.
+     */
+    default Register[] getReturnRegisters(JavaKind[] kinds, boolean includeRax) {


Rax cannot be mentioned here - it's x64 specific.

True, actually the bool is used to decide whether the first general return register should be included or not

dougxc · 2025-03-03T21:09:41Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/code/RegisterConfig.java

    Register getReturnRegister(JavaKind kind);

+    /**
+     * Gets the registers to be used for returning multiple values e.g. used for a scalarized inline object.


Needs more precise specification. How does kinds relate to the returned value? Are they the same length even when longs and doubles are involved? Providing an example can help.

Only needed to decide if the value is put into a regular or xmm register, I will add a comment. But it is just the same as with getReturnRegister just using multiple kinds.

dougxc · 2025-03-04T07:53:32Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaMethod.java

 */
 package jdk.vm.ci.meta;

+import jdk.vm.ci.hotspot.HotSpotSignature;


Cannot reference HotSpot specific types in VM agnostic parts of the API.

Ah yes the function that uses it can be deleted anyway (was never used actually).

dougxc

Are all the test changes ready for review by the Valhalla team?

dougxc · 2025-03-04T07:55:05Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaMethod.java

+     * @param index the index of a formal parameter in the signature
+     * @return the instance fields as types including the is not null type
+     */
+    default JavaType[] getScalarizedParameter(int index) {


We should List instead of arrays for new method, especially now that there are convenient ways to create immutable lists with the List.of(...) static methods.

Sounds reasonable

dougxc · 2025-03-04T07:56:44Z

test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestGetfieldChains.java


        final Scenario[] scenarios = {
-                new Scenario(0,
+                           /*new Scenario(0,


Commented out code will have to be removed or uncommented before merging.

dougxc · 2025-06-23T09:25:45Z

Please open a new version of this PR (in draft state) that targets https://github.com/openjdk/valhalla/tree/lworld instead of https://github.com/MichaelHaas99/valhalla/tree/lworld. The PR description should describe what state the changes are in and what needs to be done before it's ready for taking out of draft state.

…register config.

…doc.

MichaelHaas99 force-pushed the mh/GR-57655 branch 3 times, most recently from 7b37755 to 07ff702 Compare October 9, 2024 05:23

MichaelHaas99 force-pushed the mh/GR-57655 branch from bc0a7c3 to ef71950 Compare October 26, 2024 16:02

MichaelHaas99 requested a review from davleopo October 28, 2024 10:44

davleopo reviewed Oct 28, 2024

View reviewed changes

MichaelHaas99 force-pushed the mh/GR-57655 branch 2 times, most recently from 5dbf959 to 543708d Compare November 11, 2024 07:28

MichaelHaas99 pushed a commit that referenced this pull request Nov 13, 2024

8339794: Open source closed choice tests #1

5e5942a

Reviewed-by: jdv, prr

MichaelHaas99 pushed a commit that referenced this pull request Nov 13, 2024

8340687: Open source closed frame tests #1

47fcf5a

Reviewed-by: aivanov

MichaelHaas99 force-pushed the mh/GR-57655 branch from ddfe4c6 to 31f10f3 Compare February 22, 2025 17:46

dougxc reviewed Mar 3, 2025

View reviewed changes

dougxc reviewed Mar 4, 2025

View reviewed changes

MichaelHaas99 force-pushed the mh/GR-57655 branch from 1754167 to fdb1690 Compare March 11, 2025 10:15

MichaelHaas99 force-pushed the mh/GR-57655 branch from 4bf3091 to 6115652 Compare September 26, 2025 16:27

MichaelHaas99 added 2 commits September 26, 2025 20:42

Initial JVMCI support for Valhalla.

4e29d89

Rebase corrections.

5704b56

MichaelHaas99 force-pushed the mh/GR-57655 branch 2 times, most recently from fc4d873 to d1331cc Compare September 26, 2025 21:13

MichaelHaas99 added 9 commits September 26, 2025 23:13

Distinguish between declared and non-declared fields.

d1331cc

Combine computeInstanceFields and computerStaticFields.

cbe5ed0

Provide an interface for the new array layouts.

305ff92

Provide JVMCI types for different array layouts.

2c52b4d

Provide array properties over JVMCI.

f83e06c

Correct null marker.

0b7c932

Rename nullfree to nullrestricted. Some refactoring.

33424ac

Make interface methods non-default.

1a3395f

Refactoring.

5fb6d11

MichaelHaas99 added 16 commits October 16, 2025 22:12

Remove redundant null marker methods from field classes.

5c786a6

Add return convention in register config for aarch64. Refactoring in …

7db92dc

…register config.

Move methods from HotSpotResolvedObjectType to ResolvedJavaType. Add …

c4a2688

…doc.

Add javadoc.

97d99fb

Also save the layout properties for virtual arrays.

b8d0785

Rename inline type to value class.

b0e7279

Correct holder class logic. Add javadoc.

b3742c7

Provide the strict property of a field.

f2107df

Provide some properties of heap flattening.

4e96bfb

Rename runtime calls for the flat array load and store operation.

c800c07

Refactoring.

65e77f6

Refactoring.

96c3862

Refactoring of method attachement.

ac22767

Refactoring.

09b550d

Correct wrong method name.

d89de3b

Refactoring.

8093a98

		@@ -0,0 +1,19 @@
		package jdk.vm.ci.hotspot;

		public interface SingleTypeEntry{

Conversation

MichaelHaas99 commented Sep 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelHaas99 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougxc commented Jun 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichaelHaas99 Mar 4, 2025 •

edited

Loading

MichaelHaas99 Mar 4, 2025 •

edited

Loading

MichaelHaas99 Mar 4, 2025 •

edited

Loading

MichaelHaas99 Mar 4, 2025 •

edited

Loading