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

AArch64: Implement read barrier evaluators #9542

Merged
merged 1 commit into from
Jun 3, 2020
Merged

AArch64: Implement read barrier evaluators #9542

merged 1 commit into from
Jun 3, 2020

Conversation

knn-k
Copy link
Contributor

@knn-k knn-k commented May 13, 2020

This commit implements read barrier evaluators for AArch64.

Signed-off-by: KONNO Kazuhiro konno@jp.ibm.com

@knn-k
Copy link
Contributor Author

knn-k commented May 14, 2020

jenkins compile alinux64 jdk11

@knn-k knn-k mentioned this pull request May 14, 2020
Closed
2 tasks
@knn-k
Copy link
Contributor Author

knn-k commented May 22, 2020

Related PR: eclipse-omr/omr#5224

@knn-k knn-k mentioned this pull request May 25, 2020
Merged
@knn-k knn-k changed the title WIP: AArch64: Implement read barrier evaluators AArch64: Implement read barrier evaluators May 26, 2020
@knn-k
Copy link
Contributor Author

knn-k commented May 26, 2020

Ready for review. #9665 depends on this PR.
@0xdaryl FYI.

@knn-k knn-k mentioned this pull request May 26, 2020
Merged
@knn-k
Copy link
Contributor Author

knn-k commented May 26, 2020

The Travis CI build failure was not caused by this PR.

[ 83%] Building ASM_NASM object runtime/compiler/CMakeFiles/j9jit.dir/x/runtime/X86ArrayTranslate.nasm.o
nasm: error: invalid warning `return-type'
type `nasm -h' for help
runtime/compiler/CMakeFiles/j9jit.dir/build.make:2287: recipe for target 'runtime/compiler/CMakeFiles/j9jit.dir/x/runtime/X86ArrayTranslate.nasm.o' failed

@knn-k
Copy link
Contributor Author

knn-k commented May 26, 2020

jenkins compile alinux64 jdk11

@knn-k
Copy link
Contributor Author

knn-k commented May 27, 2020

Try running system tests before enabling concurrent scavenger.

@knn-k
Copy link
Contributor Author

knn-k commented May 27, 2020

jenkins test sanity.system alinux64 jdk11

@knn-k
Copy link
Contributor Author

knn-k commented May 27, 2020

The failure with TestIBMJlmRemoteMemoryAuth_0 is a known issue #9047.

@knn-k
Copy link
Contributor Author

knn-k commented May 28, 2020

I manually ran the following tests for concurrent scavenger from openjdk-tests/system on cent7-aarch64-1 with my local build, and they all passed.

  • DaaLoadTest_daa1_ConcurrentScavenge
  • LambdaLoadTest_ConcurrentScavenge
  • MathLoadTest_bigdecimal_ConcurrentScavenge
  • MauveSingleThreadLoadTest_ConcurrentScavenge
  • ClassLoadingTest_ConcurrentScavenge

@knn-k
Copy link
Contributor Author

knn-k commented May 28, 2020

Minor update for avoiding compiler warnings.

0xdaryl
0xdaryl requested changes May 28, 2020
View reviewed changes
runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
deps->addPostCondition(x0Reg, TR::RealRegister::x0);
deps->addPostCondition(metaReg, TR::RealRegister::NoReg);

tempMR = new (cg->trHeapMemory()) TR::MemoryReference(node, 4, cg);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the offset "4" represent? Perhaps use a more descriptive constant name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not an offset but a length parameter.
It is meaningless, and is addressed by eclipse-omr/omr#4137 .

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated

generateLabelInstruction(cg, TR::InstOpCode::label, node, endLabel, deps);

bool needSync = (node->getSymbolReference()->getSymbol()->isSyncVolatile() && cg->comp()->target().isSMP());
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the comp variable that you cached earlier.

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
deps->addPostCondition(locationReg, TR::RealRegister::x1); // TR_softwareReadBarrier helper needs this in x1.
deps->addPostCondition(evacuateReg, TR::RealRegister::NoReg);
deps->addPostCondition(x0Reg, TR::RealRegister::x0);
deps->addPostCondition(metaReg, TR::RealRegister::NoReg);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to assign metaReg to a NoReg dependence (i.e., any available real register)? Shouldn't this be the actual real register representing the vmThread?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It came from the p version.

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
TR::Register *locationReg = cg->allocateRegister();
TR::Register *evacuateReg = cg->allocateRegister();
TR::Register *x0Reg = cg->allocateRegister();
TR::Register *metaReg = cg->getMethodMetaDataRegister();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call this metaDataReg or more conventionally vmThreadReg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to vmThreadReg.

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
generateLabelInstruction(cg, TR::InstOpCode::label, node, startLabel);

generateTrg1MemInstruction(cg, TR::InstOpCode::ldrimmw, node, evacuateReg,
new (cg->trHeapMemory()) TR::MemoryReference(metaReg, cg->comp()->fej9()->thisThreadGetEvacuateBaseAddressOffset(), cg));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use comp here instead of cg->comp().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to comp, here and other locations.

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
generateConditionalBranchInstruction(cg, TR::InstOpCode::b_cond, node, endLabel, TR::CC_LT);

generateTrg1MemInstruction(cg, TR::InstOpCode::ldrimmw, node, evacuateReg,
new (cg->trHeapMemory()) TR::MemoryReference(metaReg, cg->comp()->fej9()->thisThreadGetEvacuateTopAddressOffset(), cg));
Copy link
Contributor

Choose a reason for hiding this comment

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

Use comp here

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
generateSynchronizationInstruction(cg, TR::InstOpCode::dsb, node, 0xF); // dsb SY
}

cg->insertPrefetchIfNecessary(node, objReg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? What are you prefetching here and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It came from the p version. Removing.

runtime/compiler/aarch64/codegen/J9TreeEvaluator.cpp Outdated
}

static TR::Register *
aGenerateSoftwareReadBarrier(TR::Node *node, TR::CodeGenerator *cg)
Copy link
Contributor

Choose a reason for hiding this comment

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

The DNA of this method is about 80-90% identical to the iGenerateSoftwareReadBarrier method. Is there any way you could combine them to save some code space?

Copy link
Contributor

Choose a reason for hiding this comment

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

Most of my comments in iGenerateSoftwareReadBarrier apply to this function as well so I'm not going to repeat them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged the two functions into generateSoftwareReadBarrier(), which takes bool isArdbari as its third argument.

@knn-k
AArch64: Implement read barrier evaluators
20bf33d 
This commit implements read barrier evaluators for AArch64.

Signed-off-by: KONNO Kazuhiro <konno@jp.ibm.com>
@knn-k
Copy link
Contributor Author

knn-k commented May 29, 2020

Reflected the review comments.

@knn-k
Copy link
Contributor Author

knn-k commented May 29, 2020

jenkins compile alinux64 jdk11

@knn-k
Copy link
Contributor Author

knn-k commented May 29, 2020

I ran some of the load tests (mentioned above) with both compressed refs and large heap builds, and saw no failures.

@0xdaryl 0xdaryl self-assigned this Jun 3, 2020
@0xdaryl 0xdaryl merged commit 505be67 into eclipse-openj9:master Jun 3, 2020
@knn-k knn-k deleted the aarch64swReadBarrier branch June 3, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xdaryl 0xdaryl 0xdaryl approved these changes

Assignees

@0xdaryl 0xdaryl

Labels
arch:aarch64 comp:jit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@knn-k @0xdaryl