8264957: Cleanup unused array Type::dual_type #3410

XiaohongGong · 2021-04-09T10:04:10Z

This is a bug fix for [1] which adds a new vector mask type. The new added TYPE "VectorMask" is inserted into enum TYPES, while the array "Type::dual_type" is not updated. This makes the array elements are not aligned with TYPES.

I met the following crash due to this issue when I was working on the masking feature support on panama-vector:

 Internal Error (/home/xiagon01/code/panama-vector/src/hotspot/share/opto/type.hpp:1727), pid=104432, tid=104449
 # assert(_base >= AnyPtr && _base <= KlassPtr) failed: Not a pointer

Adding a value like other vector types for the "VectorMask" in the array "dual_type" can fix it.

[1] https://bugs.openjdk.java.net/browse/JDK-8262355

Tested with tier1 and jdk:tier3

Progress

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

Issue

JDK-8264957: Cleanup unused array Type::dual_type

Reviewers

Jie Fu (@DamonFool - Committer)
Nils Eliasson (@neliasso - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3410

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

Using diff file

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

bridgekeeper · 2021-04-09T10:05:16Z

👋 Welcome back xgong! 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 · 2021-04-09T10:07:01Z

@XiaohongGong The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2021-04-09T10:11:40Z

Webrevs

01: Full - Incremental (28df8046)
00: Full (b1c68018)

DamonFool

This sounds reasonable to me.
Thanks.

neliasso

Could you please add an assert so that we don't make this mistake again.

XiaohongGong · 2021-04-12T09:27:01Z

Thanks for your review @DamonFool @neliasso ! It seems the array Type::dual_type is not used anywhere currently. The same definition for dual_type is defined in https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/type.hpp#L140 and used in type.cpp, which uses Type::dual_type before. So I think the whole definition can be totally removed. The issue I met might not be related to it. I will have a test and remove it if everything works well. Thanks!

XiaohongGong · 2021-04-13T06:36:27Z

Hi, the array Type:;dual_type is totally removed. Tested hotspot:hotspot_all_no_apps, langtools:tier1, jdk:jdk_core and jdk:tier3 internally. Would you mind having a look at it again @DamonFool @neliasso ? Thanks so much!

DamonFool · 2021-04-13T06:59:46Z

I didn't find the usage of that array either.
So the change looks fine to me.

I'm afraid the JBS needs to be updated as a cleanup issue.
Thanks.

XiaohongGong · 2021-04-13T07:03:14Z

I didn't find the usage of that array either.
So the change looks fine to me.

I'm afraid the JBS needs to be updated as a cleanup issue.
Thanks.

Seems reasonable. I will update the JBS and the PR title. Thanks!

DamonFool

Thanks for your update.

TobiHartmann

Looks good to me.

openjdk · 2021-04-13T07:36:40Z

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

8264957: Cleanup unused array Type::dual_type

Reviewed-by: jiefu, neliasso, 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 45 new commits pushed to the master branch:

954b9a1: 8264795: IGV: Upgrade NetBeans platform
f2f7aa3: 8262291: Refactor reserve_memory_special_huge_tlbfs
008fc75: 8264224: Add macosx-aarch64 to Oracle build configurations
f4e6395: 8264190: Harden TLS interop tests
18bec9c: 8265084: [BACKOUT] 8264954: unified handling for VectorMask object re-materialization during de-optimization
9dd9625: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices
1ee80e0: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
d84a7e5: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records
714ae54: 8258788: incorrect response to change in window insets [lanai]
f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
... and 35 more: https://git.openjdk.java.net/jdk/compare/951f277a71bfa5bd3a3934a8ccd66dfc25bae004...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@DamonFool, @neliasso, @TobiHartmann) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

XiaohongGong · 2021-04-13T07:37:07Z

Thanks for the review @DamonFool @TobiHartmann !

XiaohongGong · 2021-04-13T07:42:47Z

Thanks for the review @neliasso !

XiaohongGong · 2021-04-13T09:44:16Z

/integrate

openjdk · 2021-04-13T09:45:05Z

@XiaohongGong
Your change (at version 28df804) is now ready to be sponsored by a Committer.

nsjian · 2021-04-13T09:57:15Z

/sponsor

openjdk · 2021-04-13T09:59:11Z

@nsjian @XiaohongGong Since your change was applied there have been 45 commits pushed to the master branch:

954b9a1: 8264795: IGV: Upgrade NetBeans platform
f2f7aa3: 8262291: Refactor reserve_memory_special_huge_tlbfs
008fc75: 8264224: Add macosx-aarch64 to Oracle build configurations
f4e6395: 8264190: Harden TLS interop tests
18bec9c: 8265084: [BACKOUT] 8264954: unified handling for VectorMask object re-materialization during de-optimization
9dd9625: 8263763: Synthetic constructor parameters of enum are not considered for annotation indices
1ee80e0: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
d84a7e5: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records
714ae54: 8258788: incorrect response to change in window insets [lanai]
f479437: 8265082: test/hotspot/jtreg/gc/g1/TestG1SkipCompaction.java fails validate-source
... and 35 more: https://git.openjdk.java.net/jdk/compare/951f277a71bfa5bd3a3934a8ccd66dfc25bae004...master

Your commit was automatically rebased without conflicts.

Pushed as commit 1935655.

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

8264957: Type::dual_type array is not aligned with enum TYPES

b1c6801

openjdk bot added the rfr Pull request is ready for review label Apr 9, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 9, 2021

DamonFool approved these changes Apr 9, 2021

View reviewed changes

neliasso suggested changes Apr 9, 2021

View reviewed changes

Remove array Type::dual_type

28df804

XiaohongGong changed the title ~~8264957: Type::dual_type array is not aligned with enum TYPES~~ 8264957: Cleanup unused array Type::dual_type Apr 13, 2021

DamonFool approved these changes Apr 13, 2021

View reviewed changes

TobiHartmann approved these changes Apr 13, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 13, 2021

neliasso approved these changes Apr 13, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 13, 2021

openjdk bot closed this Apr 13, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 13, 2021

XiaohongGong deleted the JDK-8264957 branch September 16, 2022 01:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8264957: Cleanup unused array Type::dual_type #3410

8264957: Cleanup unused array Type::dual_type #3410

XiaohongGong commented Apr 9, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 9, 2021

openjdk bot commented Apr 9, 2021

mlbridge bot commented Apr 9, 2021 •

edited

Loading

DamonFool left a comment

neliasso left a comment

XiaohongGong commented Apr 12, 2021

XiaohongGong commented Apr 13, 2021

DamonFool commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

DamonFool left a comment

TobiHartmann left a comment

openjdk bot commented Apr 13, 2021 •

edited

Loading

XiaohongGong commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

nsjian commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

8264957: Cleanup unused array Type::dual_type #3410

8264957: Cleanup unused array Type::dual_type #3410

Conversation

XiaohongGong commented Apr 9, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 9, 2021

openjdk bot commented Apr 9, 2021

mlbridge bot commented Apr 9, 2021 • edited Loading

Webrevs

DamonFool left a comment

Choose a reason for hiding this comment

neliasso left a comment

Choose a reason for hiding this comment

XiaohongGong commented Apr 12, 2021

XiaohongGong commented Apr 13, 2021

DamonFool commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

DamonFool left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 13, 2021 • edited Loading

XiaohongGong commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

XiaohongGong commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

nsjian commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

XiaohongGong commented Apr 9, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 9, 2021 •

edited

Loading

openjdk bot commented Apr 13, 2021 •

edited

Loading