Fix ICF assert in pdcmpgt vector evaluator #3330
Conversation
The vector registers in question are marked as part of the ICF which is certainly not the case. RA will assert that we are trying to assign registers within ICF. To alleviate this we add a label right after the vector compare instruction and set ICF on the label. Signed-off-by: Filip Jeremic <[email protected]>
|
@NigelYiboYu could I please get a review? |
| break; | ||
| default: | ||
| TR_ASSERT(0, "Unrecognized op code in pd cmp vector evaluator helper."); | ||
| } | ||
|
|
||
| cursor = generateLoad32BitConstant(cg, node, 0, resultReg, true); |
There was a problem hiding this comment.
I actually think we should assert inside of these APIs that can take in dependencies for ICF. i.e. if you have ICF start already marked and you call this API and you don't pass in dependencies then assert because you're doing something wrong. This API can potentially allocate a register depending on the constant being loaded and that register has to be added to the dependencies. Food for thought.
|
Jenkins test sanity zlinux JDK8 |
|
@fjeremic is this is must fix for the 0.11 release? Does it / will it affect zLinux? |
This is a must fix for 0.11.0. It will affect Linux on Z (only on z14). |
| @@ -2701,8 +2701,8 @@ J9::Z::TreeEvaluator::pdcmpVectorEvaluatorHelper(TR::Node *node, TR::CodeGenerat | |||
| generateRRInstruction(cg, TR::Compiler->target.is64Bit() ? TR::InstOpCode::XGR : TR::InstOpCode::XR, node, resultReg, resultReg); | |||
| generateLoad32BitConstant(cg, node, 1, resultReg, true); | |||
|
|
|||
| TR::RegisterDependencyConditions* deps = new(cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 1, cg); | |||
| TR::LabelSymbol* doneCompareLabel = TR::LabelSymbol::create(cg->trHeapMemory(), cg); | |||
| TR::RegisterDependencyConditions* deps = new(cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 2, cg); | |||
There was a problem hiding this comment.
Why 2 post dependencies? I only see one call of addPostCondition.
There was a problem hiding this comment.
Is it because generateLoad32BitConstant can potentially allocate another register?
There was a problem hiding this comment.
The implicit dependency changes in APIs like generateLoad32BitConstant() can be tricky.. How do we know the number of dependencies they are adding?
There was a problem hiding this comment.
Is it because
generateLoad32BitConstantcan potentially allocate another register?
Yes, this is precisely it.
There was a problem hiding this comment.
The implicit dependency changes in APIs like generateLoad32BitConstant() can be tricky.. How do we know the number of dependencies they are adding?
This is a problem today. I can't fix the issue at the moment but I opened one a long time ago that we need to have dynamically allocated register dependencies. It's absurd to expect developers do count them manually when it costs basically nothing to just allocate them dynamically.
| @@ -2701,8 +2701,8 @@ J9::Z::TreeEvaluator::pdcmpVectorEvaluatorHelper(TR::Node *node, TR::CodeGenerat | |||
| generateRRInstruction(cg, TR::Compiler->target.is64Bit() ? TR::InstOpCode::XGR : TR::InstOpCode::XR, node, resultReg, resultReg); | |||
| generateLoad32BitConstant(cg, node, 1, resultReg, true); | |||
|
|
|||
| TR::RegisterDependencyConditions* deps = new(cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 1, cg); | |||
| TR::LabelSymbol* doneCompareLabel = TR::LabelSymbol::create(cg->trHeapMemory(), cg); | |||
| TR::RegisterDependencyConditions* deps = new(cg->trHeapMemory()) TR::RegisterDependencyConditions(0, 2, cg); | |||
There was a problem hiding this comment.
The implicit dependency changes in APIs like generateLoad32BitConstant() can be tricky.. How do we know the number of dependencies they are adding?
|
@pshipton do you want to be the committer for this one? I see we have not split the 0.11.0 branch so I assume I don't have to double deliver? Edit: FYI Jenkins tests were already launched and have passed. |
|
Correct, the branch has not yet occurred. |
The vector registers in question are marked as part of the ICF which is
certainly not the case. RA will assert that we are trying to assign
registers within ICF. To alleviate this we add a label right after the
vector compare instruction and set ICF on the label.
Signed-off-by: Filip Jeremic [email protected]