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

8265244: Remove useless comparation in LibraryCallKit::inline_vector_convert() #3507

Closed
wants to merge 2 commits into from

Conversation

@Wanghuang-Huawei
Copy link

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 15, 2021

  • aarch64 can only accept VectorReinterpret with 64/128 bits.
  • I will fix this bug by adding a rule for VectorReinterpret in match_rule_supported_vector
  • after changing note with @nsjian and @XiaohongGong , I think that two checks in inline_vector_conver is useless now. However, these checks impact other cpus, so I need more reviewers.

Progress

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

Issue

  • JDK-8265244: Remove useless comparation in LibraryCallKit::inline_vector_convert()

Contributors

  • Wang Huang <whuang@openjdk.org>
  • Ai Jiaming <aijiaming1@huawei.com>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3507

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 15, 2021

👋 Welcome back whuang! 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.

Loading

@openjdk openjdk bot added the rfr label Apr 15, 2021
@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 15, 2021

/contributor add Wang Huang whuang@openjdk.org
/contributor add Ai Jiaming aijiaming1@huawei.com

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@Wanghuang-Huawei 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@Wanghuang-Huawei
Contributor Wang Huang <whuang@openjdk.org> successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@Wanghuang-Huawei
Contributor Ai Jiaming <aijiaming1@huawei.com> successfully added.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 15, 2021

Webrevs

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 15, 2021

/test

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 15, 2021

@Wanghuang-Huawei you need to get approval to run the tests in tier1 for commits up until f65c867

Loading

@openjdk openjdk bot added the test-request label Apr 15, 2021
@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 17, 2021

@iwanowww Can you do me a favor to review this patch? Thank you.

Loading

Copy link
Contributor

@neliasso neliasso left a comment

Please change the bug and PR title to something that describes the problem or solution.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 29, 2021

Please change the bug and PR title to something that describes the problem or solution.

Thank you for your review. I will change that.

Loading

@Wanghuang-Huawei Wanghuang-Huawei changed the title 8265244: assert(false) failed: bad AD file 8265244: Remove useless comparation in LibraryCallKit::inline_vector_convert() Apr 29, 2021
@@ -2401,6 +2401,11 @@ const bool Matcher::match_rule_supported_vector(int opcode, int vlen, BasicType
} else { // NEON
// Special cases
switch (opcode) {
case Op_VectorReinterpret:
if (bit_size != 64 && bit_size != 128) {
Copy link
Contributor

@XiaohongGong XiaohongGong Apr 29, 2021

Choose a reason for hiding this comment

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

bit_size < 64 ?

Loading

Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei Apr 29, 2021

Choose a reason for hiding this comment

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

Reinterpret's size should bit_size == 64 || bit_size == 128

Loading

*/

public class TestCast4STo2I {
static final VectorSpecies<Short> SPECIESs = ShortVector.SPECIES_64;
Copy link
Contributor

@XiaohongGong XiaohongGong Apr 29, 2021

Choose a reason for hiding this comment

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

SPECIES_S?

Loading


public class TestCast4STo2I {
static final VectorSpecies<Short> SPECIESs = ShortVector.SPECIES_64;
static final VectorSpecies<Integer> SPECIESi = IntVector.SPECIES_64;
Copy link
Contributor

@XiaohongGong XiaohongGong Apr 29, 2021

Choose a reason for hiding this comment

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

ditto

Loading

for (int i = 0; i < SIZE; i++) {
System.out.print(ai[i] + ", ");
}
Copy link
Contributor

@XiaohongGong XiaohongGong Apr 29, 2021

Choose a reason for hiding this comment

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

It's better to add the correctness testing as well to make sure the results are right.

Loading

Copy link
Contributor

@theRealAph theRealAph left a comment

Please change the title of this to

8265244: AArch64: Remove useless comparation in LibraryCallKit::inline_vector_convert()

since it affects the AArch64 port. This makes the logs and the mailing lists much easier to read.

Thanks.

Loading

@Wanghuang-Huawei
Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei commented Apr 30, 2021

Please change the title of this to

8265244: AArch64: Remove useless comparation in LibraryCallKit::inline_vector_convert()

since it affects the AArch64 port. This makes the logs and the mailing lists much easier to read.

Thanks.

Thank you for your suggestion. There is only one question here:

the comparation

int num_elem_for_cast = MIN2(num_elem_from, Matcher::max_vector_size(elem_bt_to));

which will be removed here can effect all cpus.

Loading

@@ -1450,7 +1450,7 @@ bool LibraryCallKit::inline_vector_convert() {
// Since input and output number of elements are not consistent, we need to make sure we
// properly size. Thus, first make a cast that retains the number of elements from source.
// In case the size exceeds the arch size, we do the minimum.
Copy link
Contributor

@theRealAph theRealAph Apr 30, 2021

Choose a reason for hiding this comment

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

If you're going to remove the MIN2 you must also change this comment.

Loading

Copy link
Author

@Wanghuang-Huawei Wanghuang-Huawei Apr 30, 2021

Choose a reason for hiding this comment

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

Sure. I will remove that in next commit.

Loading

@@ -1469,7 +1469,7 @@ bool LibraryCallKit::inline_vector_convert() {
} else if (num_elem_from > num_elem_to) {
// Since number elements from input is larger than output, simply reduce size of input (we are supposed to
// drop top elements anyway).
Copy link
Contributor

@theRealAph theRealAph Apr 30, 2021

Choose a reason for hiding this comment

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

This one too?

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Mailing list message from Andrew Haley on hotspot-compiler-dev:

On 4/30/21 3:26 AM, Wang Huang wrote:

There is only one question here:

the comparation

int num_elem_for_cast = MIN2(num_elem_from, Matcher::max_vector_size(elem_bt_to));

which will be removed here can effect all cpus.

Perhaps so. However, you need to do something with the title because this patch
does not simply remove a useless comparison. You might make life easier for
everyone by splitting this patch into two parts.

--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 28, 2021

@Wanghuang-Huawei 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!

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 25, 2021

@Wanghuang-Huawei This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants