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

Change arraycmp length child to 64 bits #7313

Merged
merged 1 commit into from
Jun 12, 2024

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
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.

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.

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).

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.

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

@r30shah
Copy link
Contributor

r30shah commented Jun 7, 2024

@Spencer-Comin In the last launched build there seems to be failures in riscv. Can you check?

@Spencer-Comin
Copy link
Contributor Author

The riscv failure looks like an infra failure

...
10:37:07  Still waiting to schedule task
10:37:07  All nodes of label ‘compile:riscv64:cross` are offline
18:36:52  Cancelling nested steps due to timeout
...

@r30shah
Copy link
Contributor

r30shah commented Jun 7, 2024

jenkins build all

@r30shah
Copy link
Contributor

r30shah commented Jun 10, 2024

Launched build with riscv64 still failed with All nodes of label ‘[compile:riscv64:cross](https://ci.eclipse.org/omr/label/compile%3Ariscv64%3Across/)’ are offline.

@IBMJimmyk / @zl-wang can we get your review for P changes as well ?

@IBMJimmyk
Copy link
Contributor

I looked over the P changes inside compiler/p/codegen/OMRTreeEvaluator.cpp and they all look good.

@r30shah
Copy link
Contributor

r30shah commented Jun 11, 2024

Thanks @IBMJimmyk . @hzongaro / @0xdaryl I think this one is goof to merge. Can we request you to review/merge these changes ?

@hzongaro
Copy link
Member

Changes have been reviewed by people familiar with the code generation for the various platforms. Merging.

@hzongaro hzongaro merged commit 2acfd24 into eclipse:master Jun 12, 2024
15 of 18 checks passed
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.

Use 64 bits uniformly for length child of arraycmp opcode
5 participants