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

8275201: C2: hide klass() accessor from TypeOopPtr and typeKlassPtr subclasses #6717

Closed
wants to merge 8 commits into from

Conversation

rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Dec 6, 2021

Outside the type system code itself, c2 usually assumes that a
TypeOopPtr or a TypeKlassPtr's java type is fully represented by its
klass(). To have proper support for interfaces, that can't be true as
a type needs to be represented by an instance class and a set of
interfaces. This patch hides the klass() accessor of
TypeOopPtr/TypeKlassPtr and reworks c2 code that relies on it in a way
that makes that code suitable for proper interface support in a
subsequent change. This patch doesn't add proper interface support yet
and is mostly refactoring. "Mostly" because there are cases where the
previous logic would use a ciKlass but the new one works with a
TypeKlassPtr/TypeInstPtr which carries the ciKlass and whether the
klass is exact or not. That extra bit of information can sometimes
help and so could result in slightly different decisions.

To remove the klass() accessors, the new logic either relies on:

  • new methods of TypeKlassPtr/TypeInstPtr. For instance, instead of:
    toop->klass()->is_subtype_of(other_toop->klass())
    the new code is:
    toop->is_java_subtype_of(other_toop)

  • variants of the klass() accessors for narrower cases like
    TypeInstPtr::instance_klass() (returns _klass except if _klass is an
    interface in which case it returns Object),
    TypeOopPtr::unloaded_klass() (returns _klass but only when the klass
    is unloaed), TypeOopPtr::exact_klass() (returns _klass but only when
    the type is exact).

When I tested this patch, for most changes in this patch, I had the
previous logic, the new logic and a check that verified that they
return the same result. I ran as much testing as I could that way.

/cc hotspot-compiler


Progress

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

Issue

  • JDK-8275201: C2: hide klass() accessor from TypeOopPtr and typeKlassPtr subclasses

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6717

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 6, 2021

👋 Welcome back roland! 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
Copy link

openjdk bot commented Dec 6, 2021

@rwestrel 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 JDK-8275201
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch hotspot-compiler hotspot-compiler-dev@openjdk.org labels Dec 6, 2021
@openjdk
Copy link

openjdk bot commented Dec 6, 2021

@rwestrel
The hotspot-compiler label was successfully added.

@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 6, 2021
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 6, 2021
@mlbridge
Copy link

mlbridge bot commented Dec 6, 2021

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 3, 2022

@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rwestrel
Copy link
Contributor Author

rwestrel commented Jan 5, 2022

comment to keep alive.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 2, 2022

@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rwestrel
Copy link
Contributor Author

rwestrel commented Feb 2, 2022

Another comment to keep alive.

@TobiHartmann
Copy link
Member

Quickly ran this through testing, seeing build failures:

[2022-02-08T10:43:57,398Z] /System/Volumes/Data/mesos/work_dir/slaves/c82600aa-2448-475c-8c08-6f02a5b3f3af-S24420/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/d28f4559-13f6-46e3-b407-8090e4d3f411/runs/ecf578fc-ee25-4f6e-a577-ebd7c77ee02d/workspace/open/src/hotspot/share/opto/graphKit.cpp:3987:17: error: 'klass' is a protected member of 'TypeOopPtr'
[2022-02-08T10:43:57,398Z]   if (ary_type->klass()->is_array_klass()) {
[2022-02-08T10:43:57,398Z]                 ^
[2022-02-08T10:43:57,398Z] /System/Volumes/Data/mesos/work_dir/slaves/c82600aa-2448-475c-8c08-6f02a5b3f3af-S24420/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/d28f4559-13f6-46e3-b407-8090e4d3f411/runs/ecf578fc-ee25-4f6e-a577-ebd7c77ee02d/workspace/open/src/hotspot/share/opto/type.hpp:1096:20: note: declared protected here
[2022-02-08T10:43:57,398Z]   virtual ciKlass* klass() const { return _klass;     }
[2022-02-08T10:43:57,398Z]                    ^
[2022-02-08T10:43:57,398Z] /System/Volumes/Data/mesos/work_dir/slaves/c82600aa-2448-475c-8c08-6f02a5b3f3af-S24420/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/d28f4559-13f6-46e3-b407-8090e4d3f411/runs/ecf578fc-ee25-4f6e-a577-ebd7c77ee02d/workspace/open/src/hotspot/share/opto/graphKit.cpp:3988:30: error: 'klass' is a protected member of 'TypeOopPtr'
[2022-02-08T10:43:57,398Z]     BasicType bt = ary_type->klass()->as_array_klass()->element_type()->basic_type();
[2022-02-08T10:43:57,398Z]                              ^
[2022-02-08T10:43:57,398Z] /System/Volumes/Data/mesos/work_dir/slaves/c82600aa-2448-475c-8c08-6f02a5b3f3af-S24420/frameworks/1735e8a2-a1db-478c-8104-60c8b0af87dd-0196/executors/d28f4559-13f6-46e3-b407-8090e4d3f411/runs/ecf578fc-ee25-4f6e-a577-ebd7c77ee02d/workspace/open/src/hotspot/share/opto/type.hpp:1096:20: note: declared protected here
[2022-02-08T10:43:57,398Z]   virtual ciKlass* klass() const { return _klass;     }

Copy link
Contributor

@iwanowww iwanowww left a comment

Choose a reason for hiding this comment

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

Nice work, Roland! Looks good.

Except IGP-related crashes, all tests passed (hs-tier1 - hs-tier9).

Also, you need to merge it with #6952.

Some minor cleanup suggestions follow.

BasicType src_elem = src_type->klass()->as_array_klass()->element_type()->basic_type();
if (is_reference_type(src_elem)) {
BasicType src_elem = src_type->isa_aryptr()->elem()->array_element_basic_type();
if (is_reference_type(src_elem) || src_elem == T_NARROWOOP) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I find numerous new T_NARROWOOP checks distracting. Is it possible to get rid of them?
E.g., either normalize it to T_OBJECT in array_element_basic_type() or enhance is_reference_type().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I enhance is_reference_type() but it's called from so many locations that it's hard to tell if simply accepting T_NARROWOOP works for all uses. So I also added a boolean parameter.

ciKlass* klass = toop ? toop->klass() : (tkls ? tkls->klass() : NULL );
if( klass && klass->is_loaded() && klass->is_interface() ) {
const TypeInstKlassPtr *tkls = t->isa_instklassptr();
if (toop->is_interface() || tkls->is_interface()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing NULL checks on toop and tkls. It causes crashes when IGP is enabled.

if (!elem_type->is_loaded()) {
field_type = TypeInstPtr::BOTTOM;
} else if (field != NULL && field->is_static_constant()) {
// This can happen if the constant oop is non-perm.
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the comment is outdated. What is 'non-perm'? Out of PermGen?

ciInstanceKlass* iklass = NULL;
int nfields = 0;
int array_base = 0;
int element_size = 0;
BasicType basic_elem_type = T_ILLEGAL;
ciType* elem_type = NULL;
const Type *field_type = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: should be const Type* field_type.

bool is_loaded() const { return _klass->is_loaded(); }
// Instance klass, ignoring any interface
ciInstanceKlass* instance_klass() const {
if (klass()->is_loaded() && klass()->is_interface()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to normalize _klass during construction instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would work. The type of the element of new I[] is a TypeInstPtr. If I is an interface, that TypeInstPtr must not be Object. In any case, this will become cleaner with the next change where the instance klass and interfaces are recorded separately in TypeInstPtr.

@rwestrel
Copy link
Contributor Author

Thanks for the review and testing. I merged and pushed a new commit that should address your comments.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 28, 2022

@rwestrel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@rwestrel
Copy link
Contributor Author

Comment to keep alive

Copy link
Contributor

@veresov veresov left a comment

Choose a reason for hiding this comment

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

Very nice!

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed rfr Pull request is ready for review labels Apr 8, 2022
@rwestrel
Copy link
Contributor Author

Very nice!

Thanks for the review!

Copy link
Contributor

@iwanowww iwanowww left a comment

Choose a reason for hiding this comment

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

Looks very good!

Sorry for the delay with the review.

@openjdk
Copy link

openjdk bot commented May 10, 2022

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

8275201: C2: hide klass() accessor from TypeOopPtr and typeKlassPtr subclasses

Reviewed-by: vlivanov, iveresov

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 3 new commits pushed to the master branch:

  • d478958: 8286179: Node::find(int) should not traverse from new to old nodes
  • de8f4d0: 8286191: misc tests fail due to JDK-8285987
  • bf0dc4f: 8286367: riscv: riscv port is broken after JDK-8284161

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels May 10, 2022
@rwestrel
Copy link
Contributor Author

Looks very good!

Thanks for the review.

@rwestrel
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented May 11, 2022

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

  • 6586e5a: 8286459: compile error with VS2017 in continuationFreezeThaw.cpp
  • 9c25484: 8286339: compiler/c2/irTests/TestEnumFinalFold.java fails if Enum/String methods are not inlined
  • d547a70: 8286474: Drop --enable-preview from Sealed Classes related tests
  • aaeb08e: 8278123: serviceability/dcmd/vm/ClassLoaderStatsTest.java failing with java.lang.AssertionError: Should have a hidden class
  • 070a0cd: 8286551: JDK-8286460 causes tests to fail to compile in Tier2
  • dcec1d2: 8286368: Cleanup problem lists after loom integration
  • 7704eb1: 8284980: Test vmTestbase/nsk/stress/except/except010.java times out with -Xcomp -XX:+DeoptimizeALot
  • d347fc1: 8286438: Add jhsdb jstack processing without --mixed in efh
  • 61c68ab: 8285518: CDS assert: visibility cannot change between dump time and runtime
  • 52dbfa9: 8286460: Remove dependence on JAR filename in CDS tests
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/4fd79a6ad2683e4863bd4e311cb01cbc30ebf57f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 11, 2022
@openjdk openjdk bot closed this May 11, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 11, 2022
@openjdk
Copy link

openjdk bot commented May 11, 2022

@rwestrel Pushed as commit aa7ccdf.

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

@navyxliu
Copy link
Member

hi, @rwestrel ,
I see this patch uses covariant return in a few places, eg.

-  virtual const Type *cast_to_exactness(bool klass_is_exact) const;
+  virtual const TypeInstPtr* cast_to_exactness(bool klass_is_exact) const;

and

// Speculative type helper methods.
-  virtual const Type* remove_speculative() const;
+  virtual const TypeOopPtr* remove_speculative() const;

This contradicts "Avoid covariant return types." from hotspot-style. Is there any particular reason to change like that? I see that compile.cpp leverages that to improve expressiveness.

@rwestrel
Copy link
Contributor Author

Hi @navyxliu,

This contradicts "Avoid covariant return types." from hotspot-style. Is there any particular reason to change like that? I see that compile.cpp leverages that to improve expressiveness.

I missed that. I will work on a subsequent change to remove the covariant return types.

@navyxliu
Copy link
Member

hi, Roland,

I don't mean that you should remove it. The hotspot code style uses "avoid" instead of "forbid".
I am curious what we gain when we use this feature.
in some scenarios, we are operating subclass pointers. eg. in flatten_alias_type()
const TypeInstPtr *to = tj->isa_instptr();

we can save static_cast<> for them. It has better for expressiveness, doesn't it?

@rwestrel
Copy link
Contributor Author

I don't mean that you should remove it. The hotspot code style uses "avoid" instead of "forbid". I am curious what we gain when we use this feature. in some scenarios, we are operating subclass pointers. eg. in flatten_alias_type() const TypeInstPtr *to = tj->isa_instptr();

we can save static_cast<> for them. It has better for expressiveness, doesn't it?

The usual pattern in c2 code is to use of the isa_xxx()/is_xxx() methods to downcast. The covariant return types made it possible to remove some of those calls resulting in less clutter. Anyway, it was mostly convenience and it's a part of the change that can easily be adjusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
5 participants