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

Add can skip null value store check #7184

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

a7ehuo
Copy link
Contributor

@a7ehuo a7ehuo commented Nov 16, 2023

Add CanSkipNonNullableArrayNullStoreCheck for recognized method.

This change is used by the change in:

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Nov 16, 2023

@hzongaro May I ask you to review this change? Thank you!

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

I think your changes look good.

One thing I notice is that the non-helper that's used for checking for stores of null references to null-restricted arrays is called <nonNullableArrayNullStoreCheck>. Of course, the terminology that's being used in the downstream project has changed over time. I think the names of skip..., safeToSkip... and CanSkip... should be consistent with the name of the non-helper, and they should either all use nonNullableArrayNullStoreCheck or all use nullValueStoreCheckForNullRestrictedArray.

If you decide to go use with NullRestricted rather than nonNullable , it might be good to trim the length a bit - maybe something like nullRestrictedArrNullStoreCheck.

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Nov 20, 2023

the non-helper that's used for checking for stores of null references to null-restricted arrays is called .

Good point! I'll update the name to skipNonNullableArrayNullStoreCheck etc.

@a7ehuo a7ehuo force-pushed the add-skip-null-value-store-check-pr branch from 9bd959a to 0bc229a Compare November 20, 2023 18:50
Add `CanSkipNonNullableArrayNullStoreCheck` for recognized method.

Signed-off-by: Annabelle Huo <[email protected]>
@a7ehuo a7ehuo force-pushed the add-skip-null-value-store-check-pr branch from 0bc229a to aebf7dd Compare November 20, 2023 19:08
@a7ehuo
Copy link
Contributor Author

a7ehuo commented Nov 20, 2023

@hzongaro The comment is addressed. Ready for another review. Thanks!

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@hzongaro
Copy link
Member

Jenkins build all

@a7ehuo
Copy link
Contributor Author

a7ehuo commented Nov 21, 2023

x86-64 macOS test failure:

The following tests FAILED:
	  2 - TestJitBuilderAPIGenerator (Failed)
	 34 - porttest (Failed)

Linux riscv64 failure:

20:56:13  + docker build -t riscv-crosscompile .
...
20:56:13  Step 12/19 : RUN wget -O /tmp/libfakeroot_1.31-1.2_riscv64.deb 	 http://ftp.ports.debian.org/debian-ports/pool-riscv64/main/f/fakeroot/libfakeroot_1.31-1.2_riscv64.deb
20:56:13   ---> Running in 10ba7d861f73
20:56:13  �[91m--2023-11-20 20:56:11--  http://ftp.ports.debian.org/debian-ports/pool-riscv64/main/f/fakeroot/libfakeroot_1.31-1.2_riscv64.deb
20:56:13  Resolving ftp.ports.debian.org (ftp.ports.debian.org)... �[0m�[91m130.89.148.77, 2001:67c:2564:a119::77
20:56:13  Connecting to ftp.ports.debian.org (ftp.ports.debian.org)|130.89.148.77|:80... �[0m�[91mconnected.
20:56:13  �[0m�[91mHTTP request sent, awaiting response... �[0m�[91m404 Not Found
20:56:13  �[0m�[91m2023-11-20 20:56:11 ERROR 404: Not Found.
20:56:13  
20:56:13  �[0mThe command '/bin/sh -c wget -O /tmp/libfakeroot_1.31-1.2_riscv64.deb 	 http://ftp.ports.debian.org/debian-ports/pool-riscv64/main/f/fakeroot/libfakeroot_1.31-1.2_riscv64.deb' returned a non-zero code: 8
...
ERROR: script returned exit code 8
Setting status of aebf7ddc236c5fda1aa2e7803e858877775102b6 to FAILURE with url https://ci.eclipse.org/omr/job/PullRequest-linux_riscv64_cross/1278/ and message: 'Build finished. '

It looks to me an infrastructure issue.

@hzongaro
Copy link
Member

Jenkins build riscv

@hzongaro
Copy link
Member

Except for known problems, builds were all successful. Merging.

@hzongaro hzongaro merged commit 419dc95 into eclipse:master Nov 21, 2023
16 of 18 checks passed
@a7ehuo a7ehuo deleted the add-skip-null-value-store-check-pr branch March 6, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants