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.

Sign up for GitHub

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

Jump to bottom

Change arraycmp length child to 64 bits #7313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change arraycmp length child to 64 bits #7313

wants to merge 1 commit into from

Conversation

Spencer-Comin
Copy link
Contributor

Previously the arraycmp IL opcode's length child was specified as 32 bits but was inconsistent in its use. This commit changes the specification of the length child to 64 bits, and normalizes the use to 64 bits.

Closes: #6992

@Spencer-Comin Spencer-Comin mentioned this pull request Apr 17, 2024
Open
@Spencer-Comin Spencer-Comin force-pushed the arraycmp-64 branch 2 times, most recently from c758d04 to b4cf92b Compare April 17, 2024 20:43
@Spencer-Comin Spencer-Comin marked this pull request as ready for review April 25, 2024 20:31
@Spencer-Comin
Copy link
Contributor Author

x86-64 macOS failure is #7181.

Pinging @0xdaryl, @zl-wang, and @r30shah as I made tweaks to AArch64, Power, and Z codegens.

knn-k
knn-k approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@knn-k knn-k left a comment

Choose a reason for hiding this comment

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

The aarch64 changes look good to me.

@Spencer-Comin
Change arraycmp length child to 64 bits
9589a57 
Previously the arraycmp IL opcode's length child  was specified as 32 bits but
was inconsistent in its use. This commit changes the specification of the
length child to 64 bits, and normalizes the use to 64 bits.

Closes: eclipse#6992
Signed-off-by: Spencer Comin <spencer.comin@ibm.com>
@Spencer-Comin
Copy link
Contributor Author

I ran some internal sanity tests and saw some issues with i386. I made a minor change in the x86 codegen; @hzongaro could you review that?

After the x86 changes the only testing failures I am seeing seem to be infrastructure related (see internal jenkins tests here and here).

hzongaro
hzongaro approved these changes May 9, 2024
View reviewed changes
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 the x86 changes look good.

r30shah
r30shah approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@r30shah r30shah left a comment

Choose a reason for hiding this comment

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

LGTM.

@r30shah
Copy link
Contributor

r30shah commented May 9, 2024

jenkins build all

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

@knn-k knn-k knn-k approved these changes

@hzongaro hzongaro hzongaro approved these changes

@r30shah r30shah r30shah approved these changes

@Leonardo2718 Leonardo2718 Awaiting requested review from Leonardo2718

@fjeremic fjeremic Awaiting requested review from fjeremic fjeremic is a code owner

@0xdaryl 0xdaryl Awaiting requested review from 0xdaryl 0xdaryl is a code owner

@aviansie-ben aviansie-ben Awaiting requested review from aviansie-ben

@vijaysun-omr vijaysun-omr Awaiting requested review from vijaysun-omr vijaysun-omr is a code owner

@mstoodle mstoodle Awaiting requested review from mstoodle mstoodle is a code owner

Assignees
No one assigned
Labels
arch:aarch64 arch:power arch:z comp:compiler comp:test comp:tril
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use 64 bits uniformly for length child of arraycmp opcode
4 participants
@Spencer-Comin @r30shah @hzongaro @knn-k