8341784: Refactor TypeVect to use a BasicType instead of a const Type* #21414

merykitty · 2024-10-08T19:46:12Z

Hi,

This patch refactors TypeVect to use a BasicType instead of a const Type* as our current implementation. This conveys the element information in a clearer and safer manner.

Please take a look and leave your reviews,
Thanks a lot.

Progress

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

Issue

JDK-8341784: Refactor TypeVect to use a BasicType instead of a const Type* (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Jasmine Karthikeyan (@jaskarth - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21414/head:pull/21414
$ git checkout pull/21414

Update a local copy of the PR:
$ git checkout pull/21414
$ git pull https://git.openjdk.org/jdk.git pull/21414/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21414

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21414.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-08T19:46:44Z

👋 Welcome back qamai! 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 · 2024-10-08T19:46:52Z

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

8341784: Refactor TypeVect to use a BasicType instead of a const Type*

Reviewed-by: kvn, jkarthikeyan

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

4beb771: 8339538: Wrong timeout computations in DnsClient
a7d2077: 8340366: Open source several AWT Dialog tests - Batch 3
2d8fcc4: 8340193: Open source several AWT Dialog tests - Batch 1
6e01384: 8341136: Optimize StackMapGenerator::trimAndCompress
e9327b6: 8339205: Optimize StackMapGenerator
1604255: 8341619: C2: remove unused StoreCM node
e7c5bf4: 8341722: Fix some warnings as errors when building on Linux with toolchain clang
36fca5d: 8341882: [BACKOUT] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
780de00: 8051591: Test javax/swing/JTabbedPane/8007563/Test8007563.java fails
313f4a9: 8340809: Open source few more AWT PopupMenu tests
... and 60 more: https://git.openjdk.org/jdk/compare/260d4658aefe370d8994574c20057de07fd6f197...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 · 2024-10-08T19:47:41Z

@merykitty 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 · 2024-10-08T19:50:31Z

Webrevs

vnkozlov

Looks reasonable. Need to test it internally.

vnkozlov · 2024-10-08T22:52:12Z

src/hotspot/share/opto/type.cpp

 //------------------------------meet-------------------------------------------
-// Compute the MEET of two types.  It returns a new Type object.
-const Type *TypeVect::xmeet( const Type *t ) const {
+// Compute the MEET of two types. It returns a new Type object.


It never returns new type now.

vnkozlov · 2024-10-09T16:55:55Z

My testing passed.

merykitty · 2024-10-09T17:09:35Z

@vnkozlov Thanks for your reviews and testings, the latest commit addresses your concern, as well as contains some minor style changes and a removal of switch case duplication.

vnkozlov

Good.

You need second review because change is not trivial.

jaskarth

I think this is a really nice cleanup, it makes working with vectors more clear. I've added some minor stylistic fixes you could do since you're already changing these lines.

src/hotspot/share/opto/vectorIntrinsics.cpp

src/hotspot/share/opto/type.cpp

merykitty · 2024-10-10T01:07:58Z

@jaskarth Nice suggestions, I have reviewed the patch and done similar changes to nearby lines.

jaskarth

Thanks, looks good to me!

merykitty · 2024-10-10T16:49:13Z

@vnkozlov Could you re-review this, please, it seems required.

vnkozlov

Update is good.

merykitty · 2024-10-11T15:27:34Z

Thanks a lot for your reviews
/integrate

openjdk · 2024-10-11T15:28:19Z

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

6133866: 8341070: javac fails with an exception when compiling import module under source level 8
519544c: 8341909: G1: Add region index to region printer output
7c0dbf8: 8341820: Check return value of hcreate_r
ae41daf: 8341880: RISC-V: riscv_vector.h native build fails with gcc13 after JDK-8320500
ace44e5: 8341806: Gcc version detection failure on Alinux3
472db92: 8341900: Optimize DirectCodeBuilder writeBody
24eb360: 8341906: Optimize ClassFile writing BufBuffer
cd4981c: 8341257: Open source few DND tests - Set1
06f34d7: 8339651: ShenandoahPacer::setup_for_mark, ShenandoahPacer::setup_for_updaterefs and ShenandoahPacer::setup_for_evac runtime error: division by zero
97ee8bb: 8340173: Open source some Component/Panel/EventQueue tests - Set2
... and 76 more: https://git.openjdk.org/jdk/compare/260d4658aefe370d8994574c20057de07fd6f197...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-11T15:28:43Z

@merykitty Pushed as commit 7276a1b.

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

jatin-bhateja · 2024-10-11T16:16:46Z

Hi @merykitty , LGTM.

Best Regards.

merykitty · 2024-10-11T16:31:57Z

@jatin-bhateja Thanks a lot for your reviews.

eme64 · 2024-10-21T08:11:33Z

@merykitty I was out on vacation and missed this patch. Can you tell me why the long type has the letter J and not L in the vector type?

eme64 · 2024-10-21T08:20:44Z

Ah, I guess that is generally the letter we use when passing longs, like in method signatures. And that is because L is already taken for objects. Oh well, makes sense now 😆

PaulSandoz · 2024-10-21T22:13:44Z

Ah, I guess that is generally the letter we use when passing longs, like in method signatures. And that is because L is already taken for objects. Oh well, makes sense now 😆

Welcome to the in-between world of Java and the JVM :-)

merykitty added 4 commits October 9, 2024 02:34

cleanup TypeVect

b22dc6b

fix tests

a5d5de7

copyright

6dc42c3

more cleanup

78b88e4

openjdk bot added the rfr Pull request is ready for review label Oct 8, 2024

openjdk bot added the hotspot-compiler [email protected] label Oct 8, 2024

vnkozlov reviewed Oct 8, 2024

View reviewed changes

style changes

90f11d4

vnkozlov approved these changes Oct 9, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 9, 2024

jaskarth approved these changes Oct 9, 2024

View reviewed changes

more style changes

a99a743

openjdk bot removed the ready Pull request is ready to be integrated label Oct 10, 2024

jaskarth approved these changes Oct 10, 2024

View reviewed changes

vnkozlov approved these changes Oct 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 11, 2024

openjdk bot closed this Oct 11, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 11, 2024

jatin-bhateja mentioned this pull request Mar 3, 2025

8346236: Auto vectorization support for various Float16 operations #22755

Closed

3 tasks

8341784: Refactor TypeVect to use a BasicType instead of a const Type* #21414

8341784: Refactor TypeVect to use a BasicType instead of a const Type* #21414

Uh oh!

Conversation

merykitty commented Oct 8, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 8, 2024

Uh oh!

openjdk bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 8, 2024

Uh oh!

mlbridge bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Oct 9, 2024

Uh oh!

merykitty commented Oct 9, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

merykitty commented Oct 10, 2024

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 10, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

merykitty commented Oct 11, 2024

Uh oh!

openjdk bot commented Oct 11, 2024

Uh oh!

openjdk bot commented Oct 11, 2024

Uh oh!

jatin-bhateja commented Oct 11, 2024

Uh oh!

merykitty commented Oct 11, 2024

Uh oh!

eme64 commented Oct 21, 2024

Uh oh!

eme64 commented Oct 21, 2024

Uh oh!

PaulSandoz commented Oct 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

merykitty commented Oct 8, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 8, 2024 •

edited

Loading

mlbridge bot commented Oct 8, 2024 •

edited

Loading