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

L/Q again, step two #414

Closed
wants to merge 7 commits into from
Closed

L/Q again, step two #414

wants to merge 7 commits into from

Conversation

@fparain
Copy link
Collaborator

@fparain fparain commented May 18, 2021

Please review this second set of fixes for the L/Q transition.
A few changes in C1, most changes in CI.
CI always delegates to the runtime to know if a null-free array is flattened or not (to avoid duplication of the flattening decision code).

With those changes, all tests in runtime/valhalla/inlinetypes now pass with -Xcomp -XX:TieredStopAtLevel=1 (at least on Linux x64).
Some tests in compiler/valhalla/inlinetypes still fail but they will require more time to be investigated (some tests force use of C2 or expect code to be C2 compiled, but C2 is not fixed yet).

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 414

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 18, 2021

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

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2021

@fparain 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:

L/Q again, step two

Reviewed-by: thartmann

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 2 new commits pushed to the lqagain branch:

  • 6841917: Rename nullfree to null_free
  • 671a346: CDS support for InlineKlass::null_free_array_klasses() & corrected modifiers

Please see this link for an up-to-date comparison between the source branch of this pull request and the lqagain branch.
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.

➡️ To integrate this PR with the above commit message to the lqagain branch, type /integrate in a new comment.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 18, 2021

@@ -49,8 +49,7 @@ public static void main(String[] args) {
} catch(Error e) {
ex = e;
}
Asserts.assertNotNull(ex, "An ICCE should have been thrown");
Asserts.assertEquals(ex.getClass(), IncompatibleClassChangeError.class, "Error is not an ICCE");
Asserts.assertNull(ex, "No ICCE should have been thrown");
Copy link
Member

@forax forax May 18, 2021

Choose a reason for hiding this comment

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

The negation of "An ICCE should have been thrown" is "No error should have been thrown",
talking about ICCE it that context can be misleading.

Copy link
Collaborator Author

@fparain fparain May 18, 2021

Choose a reason for hiding this comment

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

Error message updated in the last commit.

Thanks,

Fred

@@ -310,12 +310,12 @@ void NewTypeArrayStub::emit_code(LIR_Assembler* ce) {
// Implementation of NewObjectArrayStub

NewObjectArrayStub::NewObjectArrayStub(LIR_Opr klass_reg, LIR_Opr length, LIR_Opr result,
CodeEmitInfo* info, bool is_inline_type) {
CodeEmitInfo* info, bool is_null_free) {
Copy link
Member

@nick-arm nick-arm May 19, 2021

Choose a reason for hiding this comment

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

Would you mind making the same change in c1_CodeStubs_aarch64.cpp to keep them in sync?

Copy link
Collaborator Author

@fparain fparain May 19, 2021

Choose a reason for hiding this comment

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

Sure, sorry for the breakage of the aarch64 platform.

Fred

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me. Added some minor comments.

@@ -475,7 +475,8 @@ ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
require_local);
if (elem_klass != NULL && elem_klass->is_loaded()) {
// Now make an array for it
return ciArrayKlass::make(elem_klass);
bool null_free_array = sym->is_Q_array_signature() && sym->char_at(1) == JVM_SIGNATURE_INLINE_TYPE;
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Why do we need the sym->char_at(1) == JVM_SIGNATURE_INLINE_TYPE check? Isn't that part of is_Q_array_signature()?

Copy link
Collaborator Author

@fparain fparain May 19, 2021

Choose a reason for hiding this comment

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

The problem with is_Q_array_signature() is that it returns true for arrays of any dimension with the last dimension element klass being a null-free primitive class. It returns true for [QFoo; but also for [[[QFoo;. But for the [[[QFoo; array, the first two dimensions are regular nullable object arrays, only the last array is a null-free array (possibly flattened).

Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Ah, right. Thanks for the explanation!

return ciObjArrayKlass::make(klass, true);
}
} else {
return ciEnv::unloaded_ciobjarrayklass();
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Can't we delegate handling the unloaded case to ciObjArrayKlass::make(klass) below? I.e. check for if (null_free && klass->is_loaded()) above.

Copy link
Collaborator Author

@fparain fparain May 19, 2021

Choose a reason for hiding this comment

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

I can change that. This was the way it was implemented initially, then I changed it to have the whole logic in a single method, but I don't have a strong opinion about this one.
Updated in the latest commit.

CLEAR_PENDING_EXCEPTION;
} else {
if (ak != NULL && ak->is_flatArray_klass()) {
is_array_flattened = true;
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Why is that boolean needed? Can't we simply return ciFlatArrayKlass::make(klass) here?

Copy link
Collaborator Author

@fparain fparain May 19, 2021

Choose a reason for hiding this comment

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

Boolean removed in the latest commit.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for updating, looks good to me (added one minor suggestion).

if (HAS_PENDING_EXCEPTION) {
CLEAR_PENDING_EXCEPTION;
} else {
if (ak != NULL && ak->is_flatArray_klass()) {
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

else and if can be merged

Copy link
Collaborator Author

@fparain fparain May 19, 2021

Choose a reason for hiding this comment

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

Code updated.

@@ -475,7 +475,8 @@ ciKlass* ciEnv::get_klass_by_name_impl(ciKlass* accessing_klass,
require_local);
if (elem_klass != NULL && elem_klass->is_loaded()) {
// Now make an array for it
return ciArrayKlass::make(elem_klass);
bool null_free_array = sym->is_Q_array_signature() && sym->char_at(1) == JVM_SIGNATURE_INLINE_TYPE;
Copy link
Member

@TobiHartmann TobiHartmann May 19, 2021

Choose a reason for hiding this comment

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

Ah, right. Thanks for the explanation!

@fparain
Copy link
Collaborator Author

@fparain fparain commented May 19, 2021

/integrate

@openjdk openjdk bot closed this May 19, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2021

@fparain Since your change was applied there have been 2 commits pushed to the lqagain branch:

  • 6841917: Rename nullfree to null_free
  • 671a346: CDS support for InlineKlass::null_free_array_klasses() & corrected modifiers

Your commit was automatically rebased without conflicts.

Pushed as commit 4aa22fb.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants