Skip to content

Fix bug with DAA enablement conditions for off-heap memory - #21561

Merged
r30shah merged 1 commit into
eclipse-openj9:masterfrom
VermaSh:daa_disable_non-vector_path
Apr 4, 2025
Merged

Fix bug with DAA enablement conditions for off-heap memory#21561
r30shah merged 1 commit into
eclipse-openj9:masterfrom
VermaSh:daa_disable_non-vector_path

Conversation

@VermaSh

@VermaSh VermaSh commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

Previously, the non-vectorized DAA path was mistakenly enabled along with the vectorized DAA path for off-heap. Now, only the vectorized DAA path is enabled as intended.

Signed-off-by: Shubham Verma shubhamv.sv@gmail.com

@VermaSh

VermaSh commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

@r30shah, @zl-wang Here's the fix for the DAA z/OS off-heap failure. I can confirm that in my local testing this fixes the failure. I'll remove the WIP tag once I have confirmed the same via vmfarm build.

@zl-wang zl-wang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks reasonable ...

// We cannot handle arraylets because hardware intrinsics act on contiguous memory
!comp()->generateArraylets() && !TR::Compiler->om.useHybridArraylets() &&
(!TR::Compiler->om.isOffHeapAllocationEnabled() || comp()->getOption(TR_DisableVectorBCD)))
(!TR::Compiler->om.isOffHeapAllocationEnabled() || !comp()->getOption(TR_DisableVectorBCD)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@VermaSh - Would it be possible to update the condition to make it more clear with the intention here - If offheap is enabled and we do not support VectorBCD then do not do optimization.

Suggested change
(!TR::Compiler->om.isOffHeapAllocationEnabled() || !comp()->getOption(TR_DisableVectorBCD)))
!(TR::Compiler->om.isOffHeapAllocationEnabled() && comp()->getOption(TR_DisableVectorBCD)))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have updated it in my latest update

@zl-wang

zl-wang commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

particularly please run the two test cases associated with the two outstanding z failures: #21433 and #21552

Previously, the non-vectorized DAA path was mistakenly enabled along with the
vectorized DAA path for off-heap. Now, only the vectorized DAA path is enabled
as intended.

Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
@VermaSh
VermaSh force-pushed the daa_disable_non-vector_path branch from d176dd9 to d07dce3 Compare April 3, 2025 15:42
@VermaSh

VermaSh commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

Personal build is good, removing the WIP tag. Failures mentioned in #21433 and #21552 are passing with this change.

@VermaSh
VermaSh marked this pull request as ready for review April 3, 2025 17:13
@VermaSh VermaSh changed the title WIP: Fix bug with DAA enablement conditions for off-heap memory Fix bug with DAA enablement conditions for off-heap memory Apr 3, 2025
@VermaSh

VermaSh commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

This is ready for review/merge

@r30shah

r30shah commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

Jenkins test sanity zlinux jdk21

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants