Enable rdbar and wrtbar Evaluators on Power #4068
Conversation
|
@0xdaryl could you please review and merge? |
|
Please fix the CI failures. |
AlenBadel
force-pushed
the
fieldwatch_rel
branch
from
bf191af to
afb07fe
Compare
This commit enables all rdbar/rdbari & wrtbar/wrtbari Evaluators on Power. Added implementations for: dwrtbar(i)Evaluator fwrtbar(i)Evaluator awrtbar(i)Evaluator Signed-off-by: Alen Badel <[email protected]>
AlenBadel
force-pushed
the
fieldwatch_rel
branch
from
2f388f4 to
6385c23
Compare
|
@0xdaryl can we merge this? |
| cg->evaluate(sideEffectNode); | ||
| cg->decReferenceCount(sideEffectNode); | ||
| // Delegate the evaluation of the remaining children and the store operation to the storeEvaluator. | ||
| return TR::TreeEvaluator::dstoreEvaluator(node, cg); |
There was a problem hiding this comment.
There is some specialized code in dstoreEvaluator that handles double stores of lbits2d opcodes. If such a scenario occurs, it looks like it tries to change the identity of the node to an lstore, have it evaluated by the lstoreEvaluator, and then changes the identity back to a dstore. However, the input node is actually the two-child write barrier node, and the recreate logic will always create a new Node (based on my understanding of the recreate logic). This is strictly incorrect as a dstore should only have one child, yet the number of children will be set to two.
I think there already is an existing problem here, but this code is making it worse because you're now creating two dummy nodes instead of one and you're creating them with more children than is allowed on a dstore node.
I am skeptical that this will cause any functional issues, but it is a leak and the node you're creating aren't strictly valid.
Correct me if I'm wrong on any of this...
There was a problem hiding this comment.
Yeah, those extra children still hanging on the nodes and not used worried me too. @cathyzhyi @dchopra001 @AlenBadel do you think that before calling regular store/load Evaluators we should set the number of children appropriately and perhaps even reset the opcode?
On another hand, who would produce such trees and not support them?
There was a problem hiding this comment.
I don't see this lbits2d type optimization in the Z evaluators for fstore and dstore.
There was a problem hiding this comment.
Please raise an issue to track the resolution of this problem. It is a pre-existing issue but not one that should be left unresolved.
| return TR::TreeEvaluator::dstoreEvaluator(node, cg); | ||
| } | ||
|
|
||
| TR::Register *OMR::Power::TreeEvaluator::fwrtbarEvaluator(TR::Node *node, TR::CodeGenerator *cg) |
There was a problem hiding this comment.
If my comment above is correct, then it also applies here for ibits2f.
|
@genie-omr build aix,plinux |
This commit enables all rdbar/rdbari & wrtbar/wrtbari Evaluators on Power.
Signed-off-by: Alen Badel [email protected]