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

8249257 Rename ValueKlass to InlineKlass #109

Closed
wants to merge 2 commits into from

Conversation

@hseigel
Copy link
Member

@hseigel hseigel commented Jul 15, 2020

Please review this tedious change. It renames class ValueKlass to InlineKlass and renames its fields and methods. It does not rename ValueArrayKlass. That will be a future change.

Also, this change does not rename things defined in gc or jit source files.

The change was tested with tiers 1 and 2 on Mac, Windows, and Linux x64, and tiers 3-5 on Linux x64.

Thanks, Harold


Progress

  • Change must not contain extraneous whitespace

Issue

Reviewers

  • Frederic Parain (fparain - Committer) ⚠️ Review applies to e832a5c

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/109/head:pull/109
$ git checkout pull/109

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 15, 2020

👋 Welcome back hseigel! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2020

@hseigel This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8249257: Rename ValueKlass to InlineKlass

Reviewed-by: fparain
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

There are currently no new commits on the lworld branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 80115902cdc42a6a4e8d8ccc7929aab07dc2ed63.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 15, 2020

Webrevs

void get_default_value_oop(Register value_klass, Register temp_reg, Register obj);
// The empty value oop, for the given ValueKlass ("empty" as in no instance fields)
// The empty value oop, for the given InlineKlass ("empty" as in no instance fields)
// get_default_value_oop with extra assertion for empty value klass
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

empty value klass -> empty inline klass

@@ -39,7 +39,7 @@
#include "oops/oop.inline.hpp"
#include "oops/typeArrayKlass.hpp"
#include "oops/typeArrayOop.inline.hpp"
#include "oops/valueKlass.hpp"
#include "oops/inlineKlass.hpp"
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

Do we really need this include?
InlineKlass is not used directly in this file, ValueArrayKlass is used and valueArrayKlass.hpp includes inlineKlass.hpp.


// Use this to return the size of an instance in heap words
// Implementation is currently simple because all value types are allocated
// Implementation is currently simple because all inline types are allocated
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

This comment might need an update, because now, not all inline types are allocated in the Java heap.
May be just specifying this size only applies to heap allocated standalone instances.

Node* klass = use->in(TypeFunc::Parms);
intptr_t ptr = igvn->type(klass)->isa_rawptr()->get_con();
clear_nth_bit(ptr, 0);
assert(Metaspace::contains((void*)ptr), "should be klass");
assert(((ValueKlass*)ptr)->contains_oops(), "returned value type must contain a reference field");
assert(((InlineKlass*)ptr)->contains_oops(), "returned value type must contain a reference field");
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

returned value type -> returned inline type

@@ -1395,10 +1393,10 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap

// restore fields of an eliminated value type array
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

value type array -> inline type array

@@ -1395,10 +1393,10 @@ static int reassign_fields_by_klass(InstanceKlass* klass, frame* fr, RegisterMap

// restore fields of an eliminated value type array
void Deoptimization::reassign_value_array_elements(frame* fr, RegisterMap* reg_map, ObjectValue* sv, valueArrayOop obj, ValueArrayKlass* vak, TRAPS) {
ValueKlass* vk = vak->element_klass();
InlineKlass* vk = vak->element_klass();
assert(vk->flatten_array(), "should only be used for flattened value type arrays");
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

flattened value type array -> flattened inline type array

@@ -2835,7 +2835,7 @@ CodeOffsets::Entries CompiledEntrySignature::c1_value_ro_entry_type() const {
void CompiledEntrySignature::compute_calling_conventions() {
// Get the (non-scalarized) signature and check for value type arguments
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

value type arguments -> inline type arguments

@@ -573,7 +573,7 @@ typedef GrowableArrayFilterIterator<SigEntry, SigEntryFilter> ExtendedSignature;

// Used for adapter generation. One SigEntry is used per element of
// the signature of the method. Value type arguments are treated
Copy link
Collaborator

@fparain fparain Jul 15, 2020

Choose a reason for hiding this comment

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

Value type argument -> Inline type argument

@fparain
Copy link
Collaborator

@fparain fparain commented Jul 15, 2020

Harold,

Thank you for doing this tedious but necessary renaming job.
Looks good to me, just a few comments, mostly about fixing comments (no need for another review).

Regards,

Fred

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jul 15, 2020

Thanks Fred! I'll fix the issues that you found before integrating the change.
Harold

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jul 15, 2020

/integrate

@openjdk openjdk bot closed this Jul 15, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 15, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2020

@hseigel
Pushed as commit 39d2ef3.

@hseigel hseigel deleted the inlineKlass branch Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants