Remove implicit sign extension assumptions from iRegStore evaluator #4103
Conversation
The `TR::iRegStore` evaluator on Z makes an implicit assumption that if the value being globally stored is a conversion of an appropriate type, for example `TR::su2i` of a `TR::cloadi` then the evaluators will have generated a full 64-bit sign extended load. This is very dangerous as an evaluator should never implicitly assume another evaluator will generate a specific instruction sequence to perform some action. Because of this implicit assumption the `TR::iRegStore` evaluator omitted the generation of an explicit sign extension and just assumed the evaluator has already done it. It then marked the register via the `setAlreadySignExtended` API that the extension has been performed. Later a `TR::i2l` conversion operation consumes the value of the `TR::su2i` and it checks whether the child register has been marked via the `alreadySignExtended` API in which case it would return true, however no sign extension was performed. This ends up being a functional problem as we could have garbage values in the high-order 32-bits which were never cleared. Rather than allowing implicit assumptions about sign extensions we leave that to the LoadExtensions optimization and perform the sign extension explicitly via an `LGFR` instruction. An example of the trees encountered is the following: ``` ------------------------------ n4469n ( 0) iRegStore GPR7 (Unsigned NeedsSignExt privatizedInlinerArg ) n4470n ( 4) su2i (in GPR_1545) (X>=0 unneededConv ) n4471n ( 0) cloadi <array-shadow>[eclipse#236 Shadow] [flags 0x80000602 0x0 ] (in GPR_1545) (cannotOverflow zeroExtendTo32BitAtSource ) n14613n ( 1) ==>aRegLoad (in *GPR_1537) (X!=0 X>=0 SeenRealReference sharedMemory ) ------------------------------ [ 0x3ff1a2e9790] LLH GPR_1545, Shadow[<array-shadow>] 0(*GPR_1537) ... ... ------------------------------ n2929n ( 0) ificmplt --> block_1286 BBStart at n13709n () n2923n ( 2) b2i (in GPR_1554) (unneededConv ) n2922n ( 0) bloadi <array-shadow>[eclipse#235 Shadow] [flags 0x80000601 0x0 ] (in GPR_1554) (cannotOverflow signExtendedTo32BitAtSource ) n14616n ( 3) ==>aRegLoad (in &GPR_1540) (X!=0 SeenRealReference sharedMemory ) n2920n ( 0) lsub (in GPR_1545) (highWordZero X>=0 cannotOverflow ) n2919n ( 0) i2l (in GPR_1545) (highWordZero X>=0 ) n4470n ( 0) ==>su2i (in GPR_1545) (X>=0 unneededConv ) n2918n ( 0) lconst -8 (X!=0 X<=0 ) n2926n ( 0) iconst 0 (X==0 X>=0 X<=0 ) n14638n ( 0) GlRegDeps () n14612n ( 3) ==>aRegLoad (in &GPR_1536) n14614n ( 2) ==>iRegLoad (in GPR_1538) (X>=0 cannotOverflow SeenRealReference ) n14616n ( 3) ==>aRegLoad (in &GPR_1540) (X!=0 SeenRealReference sharedMemory ) ------------------------------ [ 0x3ff1a2eafd0] LB GPR_1554, Shadow[<array-shadow>] 8(GPR_1545,&GPR_1540) ``` Fixes: eclipse-openj9/openj9#6248 Signed-off-by: Filip Jeremic <fjeremic@ca.ibm.com>
|
@genie-omr build zlinux,zos |
|
Is this ready to be merged now? |
It is indeed. Waiting for a committer to pick it up. |
|
|
||
| TR::Register* globalReg = NULL; | ||
|
|
||
| // If the child is an add/sub that could be used in a branch on count op, don't evaluate it here |
There was a problem hiding this comment.
Can you please clarify what these lines changed have to do with the problem described in the commit message ? This seems to be avoiding evaluating the RHS value of a reg store altogether whereas the bug that was being talked about was around some value child not doing some extension that the parent was assuming would have happened.
There was a problem hiding this comment.
This sequence is preset in the current evaluator as well. See [1]. The reason you see the change in green in the diff is because I moved it up to the top of the evaluator since the fast-path exit in this code does not rely on any other parts of the same evaluator and hence can be done first.
[1]
omr/compiler/z/codegen/OMRTreeEvaluator.cpp
Lines 12029 to 12038 in deaaf6c
There was a problem hiding this comment.
Okay I think I see how this is related. The code later was omitting evaluating some child in some cases but was setting a flag that was causing extension to be skipped. Now we omit evaluating the child early such that the flag also won't be set.
There was a problem hiding this comment.
Once I get confirmation that I've understood this right, I'll merge.
There was a problem hiding this comment.
Your understanding is spot on.
The
TR::iRegStoreevaluator on Z makes an implicit assumption that ifthe value being globally stored is a conversion of an appropriate type,
for example
TR::su2iof aTR::cloadithen the evaluators will havegenerated a full 64-bit sign extended load.
This is very dangerous as an evaluator should never implicitly assume
another evaluator will generate a specific instruction sequence to
perform some action. Because of this implicit assumption the
TR::iRegStoreevaluator omitted the generation of an explicit signextension and just assumed the evaluator has already done it. It then
marked the register via the
setAlreadySignExtendedAPI that theextension has been performed. Later a
TR::i2lconversion operationconsumes the value of the
TR::su2iand it checks whether the childregister has been marked via the
alreadySignExtendedAPI in whichcase it would return true, however no sign extension was performed.
This ends up being a functional problem as we could have garbage values
in the high-order 32-bits which were never cleared.
Rather than allowing implicit assumptions about sign extensions we
leave that to the LoadExtensions optimization and perform the sign
extension explicitly via an
LGFRinstruction.An example of the trees encountered is the following:
Fixes: eclipse-openj9/openj9#6248
Signed-off-by: Filip Jeremic fjeremic@ca.ibm.com