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

8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr #3880

Closed
wants to merge 15 commits into from

Conversation

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented May 5, 2021

This is some refactoring in another attempt to fix JDK-6312651
(Compiler should only use verified interface types for
optimization). Rather than propose the patch from:

https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-May/033803.html

as a single big patch. I've been working on splitting it. The plan is
to have this and another refactoring patch that include no change to
the way interfaces are handled as preparation. Then only, in a third
patch, interface support along the lines of the patch I proposed 2
years ago would be introduces.

This patch changes the class hierarchy of types that C2 uses and
introduces TypeInstKlassPtr/TypeAryKlassPtr that mirror
TypeInstPtr/TypeAryPtr. The motivation for this is that a single:

ciKlass* _klass;

is no longer sufficient to describe a type (a set of interfaces must
also be carried around). That's not possible with TypeKlassPtr.

The meet methods for TypeInstPtr/TypeInstKlassPtr and
TypeAryPtr/TypeAryKlassPtr are largely similar. I moved the most
complicated logic in helper methods:

meet_instptr() and meet_aryptr()

(Thanks to Vladimir I for testing the refactoring patches)

/cc hotspot-compiler


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3880

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 5, 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 openjdk bot commented May 5, 2021

@rwestrel
The hotspot-compiler label was successfully added.

@openjdk openjdk bot added the rfr label May 5, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 5, 2021

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 2, 2021

@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 rwestrel commented Jun 7, 2021

rebased to latest jdk code. Anyone for this?

@iwanowww
Copy link

@iwanowww iwanowww commented Jun 7, 2021

I started reviewing the patch.

Can you, please, elabroate on design goals of the patch?
I don't fully understand the intended relation between TypeInstKlassPtr, TypeAryKlassPtr, and TypeKlassPtr.

My observations so far:

  • Type::base() can never be Type::KlassPtr anymore: it's either InstKlassPtr or AryKlassPtr; all t->base() == Type::KlassPtr should be replaced by t->base() == Type::InstKlassPtr || t1->base() == Type::AryKlassPtr;

  • java.lang.Object is represented with InstKlassPtr (TypeInstKlassPtr::OBJECT) and as a consequence type flattening can turn TypeAryKlassPtr into TypeInstKlassPtr.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Jun 7, 2021

I started reviewing the patch.

Thanks!

Can you, please, elabroate on design goals of the patch?
I don't fully understand the intended relation between TypeInstKlassPtr, TypeAryKlassPtr, and TypeKlassPtr.

On the oop side, TypeOopPtr is the super type for TypeInstPtr and TypeAryPtr. Type*KlassPtr are intended to work the same.
To keep track of interfaces, a TypeInstPtr will need an extra field that contains the set of interfaces that it implements.
A TypeAryPtr implements some interfaces (Cloneable etc.). If its elements are objects, its elements may implement interfaces too. A TypeAryPtr has a TypeAry field that references a TypeOopPtr for object arrays. Interfaces implemented by the array elements are attached to that TypeOopPtr.

Now with only a TypeKlassPtr where do we record interfaces implemented by array elements?

The motivation of this change is to mirror the structure described above with TypeOopPtr on the TypeKlassPtr and to have somewhere to store interfaces for object arrays.

My observations so far:

  • Type::base() can never be Type::KlassPtr anymore: it's either InstKlassPtr or AryKlassPtr; all t->base() == Type::KlassPtr should be replaced by t->base() == Type::InstKlassPtr || t1->base() == Type::AryKlassPtr;

I don't think that's true: roughly TypeAryKlassPtr meets TypeInstKlassPtr would give a TypeKlassPtr (same as TypeAryPtr meets TypeInstPtr gives a TypeOopPtr).

  • java.lang.Object is represented with InstKlassPtr (TypeInstKlassPtr::OBJECT) and as a consequence type flattening can turn TypeAryKlassPtr into TypeInstKlassPtr.

That should again be the same as what's going on with TypeOopPtr/TypeInstPtr/TypeAryPtr.

@iwanowww
Copy link

@iwanowww iwanowww commented Jun 7, 2021

TypeAryKlassPtr meets TypeInstKlassPtr would give a TypeKlassPtr (same as TypeAryPtr meets TypeInstPtr gives a TypeOopPtr).

Can you point me to the relevant code, please? What I'm seeing so far is that, in constrast to TypeOopPtr, TypeKlassPtr is never instantiated at runtime and TypeKlassPtr* always points to either TypeInstKlassPtr or TypeAryKlassPtr.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Jun 8, 2021

TypeAryKlassPtr meets TypeInstKlassPtr would give a TypeKlassPtr (same as TypeAryPtr meets TypeInstPtr gives a TypeOopPtr).

Can you point me to the relevant code, please? What I'm seeing so far is that, in constrast to TypeOopPtr, TypeKlassPtr is never instantiated at runtime and TypeKlassPtr* always points to either TypeInstKlassPtr or TypeAryKlassPtr.

You're right about that (my reply was incorrect). I don't understand why it matters this said. TypeKlassPtr is for pointers to classes that are not known to be a instance class or` an array class. That might prove useful at some point.

@iwanowww
Copy link

@iwanowww iwanowww commented Jun 8, 2021

I don't understand why it matters this said. TypeKlassPtr is for pointers to classes that are not known to be a instance class or` an array class. That might prove useful at some point.

There are leftover usages of Type::KlassPtr in the code. In particular:

src/hotspot/share/opto//idealGraphPrinter.cpp:      } else if (t->base() == Type::KlassPtr) {
src/hotspot/share/opto//memnode.cpp:  } else if (tp->base() == Type::KlassPtr) {

Also, Type::category() is not adjusted for Type::InstKlassPtr/Type::AryKlassPtr.

@iwanowww
Copy link

@iwanowww iwanowww commented Jun 8, 2021

Another case is in Compile::flatten_alias_type():

  // Flatten all to bottom for now
  switch( _AliasLevel ) {
  ...
  case 1:                       // Flatten to: oop, static, field or array
    switch (tj->base()) {
    case Type::RawPtr:   tj = TypeRawPtr::BOTTOM;   break;
    case Type::AryPtr:   // do not distinguish arrays at all
    case Type::InstPtr:  tj = TypeInstPtr::BOTTOM;  break;
    case Type::KlassPtr: tj = TypeInstKlassPtr::OBJECT; break;
    case Type::AnyPtr:   tj = TypePtr::BOTTOM;      break;  // caller checks it

Type::KlassPtr should be accompanied by Type::InstKlassPtr and Type::AryKlassPtr cases, shouldn't it?

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Jun 9, 2021

Type::KlassPtr should be accompanied by Type::InstKlassPtr and Type::AryKlassPtr cases, shouldn't it?

Good catch. I fix the ones you found. They seem mostly harmless, this said.

Copy link

@iwanowww iwanowww left a comment

Overall, it looks good.

Testing results (hs-tier1 - hs-tier6) are fine.

Just a couple of minor comments follow.

@@ -377,9 +377,6 @@ static void format_helper( PhaseRegAlloc *regalloc, outputStream* st, Node *n, c
case Type::InstPtr:
st->print(" %s%d]=#Ptr" INTPTR_FORMAT,msg,i,p2i(t->isa_oopptr()->const_oop()));
break;
case Type::KlassPtr:

Choose a reason for hiding this comment

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

Why do you remove the case? I'd expect to see 2 new cases added:

   case Type::KlassPtr:
+  case Type::AryKlassPtr:
+  case Type::InstKlassPtr:

@@ -1453,7 +1453,8 @@ const TypePtr *Compile::flatten_alias_type( const TypePtr *tj ) const {
case Type::RawPtr: tj = TypeRawPtr::BOTTOM; break;
case Type::AryPtr: // do not distinguish arrays at all
case Type::InstPtr: tj = TypeInstPtr::BOTTOM; break;
case Type::KlassPtr: tj = TypeKlassPtr::OBJECT; break;
case Type::AryKlassPtr:

Choose a reason for hiding this comment

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

Similar situation here. Why don't you leave Type::KlassPtr intact and just extend it to Type::AryKlassPtr and Type::InstKlassPtr cases?

@openjdk
Copy link

@openjdk openjdk bot commented Jul 12, 2021

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

8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr

Reviewed-by: vlivanov, 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 27 new commits pushed to the master branch:

  • 8d73ee6: 8273471: Add foldmultilines to UL for stdout/err
  • c54a918: 8273691: Missing comma after 2021 in GraphemeTestAccessor.java copyright notice
  • 3d9dc8f: 8273430: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
  • 5095068: 8273675: Remove unused Universe::_verify_in_progress flag
  • fc0f854: 8246797: A convenient method to read OPTIONAL element
  • 6cf5079: 8273513: Make java.io.FilterInputStream specification more precise about overrides
  • b4b1210: 8273616: Fix trivial doc typos in the java.base module
  • 7c26ddb: 8195809: [TESTBUG] jps and jcmd -l support for containers is not tested
  • 4cfa230: 8273259: Character.getName doesn't follow Unicode spec for ideographs
  • f9b2507: 8271834: TestStringDeduplicationAgeThreshold intermittent failures on Shenandoah
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/461a467f91ba19ae35d7833b7d3e74f62f52e19c...master

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 the ready label Jul 12, 2021
@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Jul 13, 2021

@iwanowww Thanks for the review (and help along the way). I updated the change with your suggestions.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Hard to review but looks good to me. Just added some minor comments/questions.

if (klass && klass->is_loaded() && klass->is_interface()) {
st->print(" Interface:");
} else if (toop) {
if (toop) {
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

Why did you remove the interface case?

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

I brought it back as this doesn't really matter for this PR. The next change in the series removes the klass() accessor and that creates all sort of issues. In some cases where the use of klass() seems to have little value (like here, after all interfaces can't be trusted), I removed the code using the klass() rather than try to fix it.

}

inline const TypeAryKlassPtr *Type::is_aryklassptr() const {
assert( _base == AryKlassPtr, "Not a klass pointer" );
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

The extra whitespaces should be removed.

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

done.

}

inline const TypeInstKlassPtr *Type::is_instklassptr() const {
assert( _base == InstKlassPtr, "Not a klass pointer" );
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

The extra whitespaces should be removed.

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

done.

}

inline const TypeKlassPtr *Type::is_klassptr() const {
assert( _base == KlassPtr, "Not a klass pointer" );
assert( _base >= KlassPtr && _base <= AryKlassPtr, "Not a klass pointer" );
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

The extra whitespaces should be removed.

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

done.

if (elem->isa_klassptr() && !klass_is_exact) {
elem = elem->is_klassptr()->cast_to_exactness(klass_is_exact);
}
return make(klass_is_exact ? Constant : NotNull, elem, k, _offset);
}


//-----------------------------as_instance_type--------------------------------
// Corresponding type for an instance of the given class.
// It will be NotNull, and exact if and only if the klass type is exact.
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

Is this comment still valid?

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

Nice catch. Fixed.

@@ -5968,7 +5968,7 @@ bool LibraryCallKit::inline_cipherBlockChaining_AESCrypt(vmIntrinsics::ID id) {

ciInstanceKlass* instklass_AESCrypt = klass_AESCrypt->as_instance_klass();
const TypeKlassPtr* aklass = TypeKlassPtr::make(instklass_AESCrypt);
const TypeOopPtr* xtype = aklass->as_instance_type();
const TypeOopPtr* xtype = aklass->as_instance_type()->cast_to_ptr_type(TypePtr::NotNull);
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

I assume you need this because you've changed as_instance_type to no longer cast to TypePtr::NotNull. Why is that and what about other usages of as_instance_type?

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

I looked at other use of as_instance_type and found that there was not need for the cast to not null. As to why, it makes little sense to me that as_instance_type() would return NotNull. It feels error prone.

UNLOADED,
SUBTYPE,
NOT_SUBTYPE,
LCA
Copy link
Member

@TobiHartmann TobiHartmann Jul 19, 2021

Choose a reason for hiding this comment

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

Please add comments explaining the meaning of these.

Copy link
Contributor Author

@rwestrel rwestrel Jul 27, 2021

Choose a reason for hiding this comment

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

I added a comment. Let me know if it's ok.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Jul 27, 2021

Hard to review but looks good to me. Just added some minor comments/questions.

Thanks for reviewing this.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for making these changes. Looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 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-8266550
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 and removed ready labels Aug 10, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 7, 2021

@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 rwestrel commented Sep 10, 2021

I had to tweak test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java after merging with master. @iwanowww @TobiHartmann are you still ok with the change?

@openjdk openjdk bot added ready and removed merge-conflict labels Sep 10, 2021
Copy link

@iwanowww iwanowww left a comment

Still looks good to me.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Still looks good to me.

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 14, 2021

@TobiHartmann @iwanowww thanks for the re-review!

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Sep 14, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

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

  • a143372: 8273438: Enable parallelism in vmTestbase/metaspace/stressHierarchy tests
  • 86a8e55: 8273486: Zero: Handle DiagnoseSyncOnValueBasedClasses VM option
  • 8d73ee6: 8273471: Add foldmultilines to UL for stdout/err
  • c54a918: 8273691: Missing comma after 2021 in GraphemeTestAccessor.java copyright notice
  • 3d9dc8f: 8273430: Suspicious duplicate condition in java.util.regex.Grapheme#isExcludedSpacingMark
  • 5095068: 8273675: Remove unused Universe::_verify_in_progress flag
  • fc0f854: 8246797: A convenient method to read OPTIONAL element
  • 6cf5079: 8273513: Make java.io.FilterInputStream specification more precise about overrides
  • b4b1210: 8273616: Fix trivial doc typos in the java.base module
  • 7c26ddb: 8195809: [TESTBUG] jps and jcmd -l support for containers is not tested
  • ... and 19 more: https://git.openjdk.java.net/jdk/compare/461a467f91ba19ae35d7833b7d3e74f62f52e19c...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Sep 14, 2021

@rwestrel Pushed as commit 1d2458d.

💡 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
3 participants