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

Enable arraycmp and arraycmplen uniformly #7108

Merged
merged 1 commit into from
Nov 3, 2023

Conversation

Spencer-Comin
Copy link
Contributor

@Spencer-Comin Spencer-Comin commented Sep 6, 2023

For all platforms where arraycmp and arraycmplen are supported, do not call setSupportsArrayCmp(Len) when either

  • arraylets can be generated
  • the TR_DisableArrayCmp(Len) environment variable is set

See eclipse-openj9/openj9#16662 (comment)
Relies on #6983, leaving this as draft until that goes through

For all platforms where arraycmp and arraycmplen are supported, do not
set the corresponding support flags in the code generator when either
- arraylets can be generated
- the TR_DisableArrayCmp(Len) environment variable is set

Signed-off-by: Spencer Comin <[email protected]>
@Spencer-Comin
Copy link
Contributor Author

@jdmpapin @r30shah fyi

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.

Wondering if we should add third function to OMRCodeGenerator that can reset the flag for supporting arrayCmp ?
Reason I am thinking it would be beneficial to do so is that, we can then have this code in common code-gen and the platform which does not support arrayCmp can reset it.

That being said, I am OK with this set of changes, it just seems repeated change on all code-gen.

Also @VermaSh Would appreciate if you can comment on how things would be with off-heap.

@r30shah
Copy link
Contributor

r30shah commented Oct 11, 2023

@Spencer-Comin Is there anything needed to be done for this?

@VermaSh Ping for comment about off-heap.

@Spencer-Comin
Copy link
Contributor Author

Is there anything needed to be done for this?

Assuming no changes need to be made for off-heap, this is good to go

@VermaSh
Copy link
Contributor

VermaSh commented Oct 11, 2023

Sorry I missed this. From a quick glance at the array compare evaluators, all it needs is contiguously allocated arrays. So in the off heap world, we could just replace the base address with pointer to off heap contiguously allocated data portion. This would simply be generating IL to load dataAddr field from array header. The API to check if off heap is enabled hasn't been checked upstream yet so this is for now.

@jdmpapin
Copy link
Contributor

jdmpapin commented Nov 2, 2023

Jenkins build all

@jdmpapin jdmpapin merged commit 2a9a7f4 into eclipse:master Nov 3, 2023
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.

4 participants