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

WIP: Accelerate Unsafe CAS Intrinsics on Z #20308

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

IBMJimmyk
Copy link
Contributor

@IBMJimmyk IBMJimmyk commented Oct 6, 2024

Adds support for the following recognized methods:

CompareAndExchangeObject     //JDK11
CompareAndExchangeReference  //JDK17,21
CompareAndExchangeInt        //JDK11,17,21
CompareAndExchangeLong       //JDK11,17,21

The accelerated CAE code was built on top of the existing accelerated CAS support on Z. Changes are similar to previously added support on Power and X.

Edit:
Depends on: eclipse/omr#7482
Ideally merged as a pair.

Adds support for the following recognized methods:
CompareAndExchangeObject     //JDK11
CompareAndExchangeReference  //JDK17,21
CompareAndExchangeInt        //JDK11,17,21
CompareAndExchangeLong       //JDK11,17,21

The accelerated CAE code was built on top of the existing accelerated
CAS support on Z. Changes are similar to previously added support on
Power and X.

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

This is marked WIP while I get finish running through all the unit tests I wrote. But all the tests for major functionality are passing so far so I don't expect any major changes.

@hzongaro Could you take a look at this. The common changes are small in an area you've looked at recently so it should be fairly quick.

@r30shah Could you look over the Z codegen changes? Everything seems to work but I don't work when Z codegen often so I might have missed smaller details.

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.

Skimmed through the changes, few nitpicks - I think overall looks good to me, will give one final review once this moves out of WIP.

runtime/compiler/z/codegen/J9TreeEvaluator.hpp Outdated Show resolved Hide resolved
runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
return true;
}
}
// Too risky to do Long-31bit version now.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this comment close to where we only do the transformation for 64-bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not exactly sure when you want me to move this. The comment is right after the block with the comp->target().is64Bit() check.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think right before the condition for 64-bit would be the appropriate place. I would not have thought of else part for 31-bit.

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 changes look good. Just one comment on the naming of compressedValueNode versus decompressedValueNode, neither of which I'm keen on, but I also didn't offer another suggestion. . . .

runtime/compiler/z/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
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.

Looks good to me, will wait for you to remove WIP first.

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. I'll mark this as approved, assuming you will move the one comment that @r30shah mentioned regarding the absence of a 31-bit version.

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