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

WIP: Fix for incorrectly marked collected reference on X86 #3035

Closed
wants to merge 1 commit into from
Closed

WIP: Fix for incorrectly marked collected reference on X86 #3035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 5 additions & 18 deletions compiler/x/codegen/BinaryCommutativeAnalyser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2000, 2016 IBM Corp. and others
* Copyright (c) 2000, 2018 IBM Corp. and others
*
* This program and the accompanying materials are made available under
* the terms of the Eclipse Public License 2.0 which accompanies this
Expand Down Expand Up @@ -860,30 +860,17 @@ TR::Register *TR_X86BinaryCommutativeAnalyser::integerAddAnalyserImpl(TR::Node
}
else if (getCopyRegs())
{
TR::Register *tempReg;
TR::Register* tempReg = _cg->allocateRegister();
if (firstRegister->containsCollectedReference() ||
secondRegister->containsCollectedReference() ||
firstRegister->containsInternalPointer() ||
secondRegister->containsInternalPointer())
{
if (root->isInternalPointer())
if (root->isInternalPointer() && root->getPinningArrayPointer())
{
tempReg = _cg->allocateRegister();
if (root->getPinningArrayPointer())
{
tempReg->setContainsInternalPointer();
tempReg->setPinningArrayPointer(root->getPinningArrayPointer());
}
tempReg->setContainsInternalPointer();
tempReg->setPinningArrayPointer(root->getPinningArrayPointer());
}
else if (comp->generateArraylets() && root->getOpCodeValue() == TR::aiadd)
// arraylets: aiadd is technically internal pointer into spine object, but isn't marked as internal pointer
tempReg = _cg->allocateRegister();
else
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume this allocateCollectedReferenceRegister section of code is the one that was causing problems ?
Could you please elaborate on the actual problem that you are fixing ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result register (tempReg here) was incorrectly marked as a collected reference, when the aiadd/aladd node is not internal pointer. As a result, an invalid GC map entry may be introduced and causes GC assertions.
I cannot think of any valid reasons that this result could be collected; therefore, all branches are now to allocate a "non-collected" register instead of "collected" ones.

Copy link
Contributor

@vijaysun-omr vijaysun-omr Oct 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It seems we would have used an uncollected register if the isInternalPointer flag would have been "true" on this aiadd/aladd. I expect this node flag would be set on all aiadd/aladd in Java but I guess you saw some case where that flag was not set (?) If so, how did such a node originate ?

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 from Value Propagation where a TR::arraycopy gets generated. The source and destination children are TR::aladd.

Copy link
Contributor

Choose a reason for hiding this comment

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

If these aladds created by arraycopy generation in value propagation don't have the internal pointer flag set, then we should consider setting the flag and see if that fixes the failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But either way aladd shouldn't return a collected reference. It doesn't make sense to create a collected reference from [address+offset].

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, but it's preferable to fix a bug in common code if we can so that each platform does'nt have to make changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. @liqunl kindly agreed to investigate why root->isInternalPointer() answered no as a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like VP arraycopy transformation creates an aiadd/aladd without setting the flag. I'm trying to find where the node is created.

tempReg = _cg->allocateCollectedReferenceRegister();
}
else
{
tempReg = _cg->allocateRegister();
}

targetRegister = tempReg;
Expand Down
17 changes: 1 addition & 16 deletions compiler/x/codegen/BinaryEvaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,23 +789,8 @@ TR::Register *OMR::X86::TreeEvaluator::integerAddEvaluator(TR::Node *node, TR::C
(internalPointerMismatch || targetRegister->containsCollectedReference()))))
{
TR::Register *firstOperandReg = targetRegister;
targetRegister = cg->allocateRegister();

// For a non-internal pointer TR::aiadd created when merging news (for example)
// commoning is permissible across a GC point; however the register
// needs to be marked as a collected reference for GC to behave correctly;
// note that support for internal pointer TR::aiadd (e.g. array access) is not present
// still
//
if (targetRegister->containsCollectedReference() &&
(node->getOpCode().isArrayRef()) &&
!node->isInternalPointer())
{
targetRegister = cg->allocateCollectedReferenceRegister();
}
else
{
targetRegister = cg->allocateRegister();
}
// comp()->useCompressedPointers
// ladd
// ==>iu2l
Expand Down