Remove implicit sign extension assumptions from iRegStore evaluator (0.15.0) #30
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>[#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>[#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>
pshipton
approved these changes
Jul 10, 2019
|
I'll wait for the OMR promotion build https://ci.eclipse.org/openj9/view/Pipelines/job/Pipeline-OMR-Acceptance/1504/ to complete before merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Port of eclipse-omr/omr#4103
Signed-off-by: Filip Jeremic fjeremic@ca.ibm.com