search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 722 forks source link

Proposal: Deprecate Decimal Floating Point (DFP) related support #12533

Closed fjeremic closed 3 years ago

fjeremic commented 3 years ago

Background

Decimal Floating Point (DFP) support within JVM is primarily contained within the BigDecimal class. The idea was that we could implement a series of APIs which detect the presence of DFP support from the hardware the JVM is running on and attempt to exploit DFP instructions via the JIT compiler on custom crafted methods which perform DFP arithmetic. A canonical example of DFP arithmetic may be found in BigDecimal operations, some of which may be reduced to DFP operations in certain scenarios.

Unfortunately much of this BigDecimal source code is not available in OpenJ9 or OpenJDK. It was historically exploited by the IBM JDK. Moreover DFP support has been disabled by default for the better part of the last decade.

This issue is a proposal to deprecate DFP support within OpenJ9.

Reasons to deprecate DFP

  1. DFP support is disabled by default and has been for a long time. The three flags governing whether it is enabled or not are:

    • -Xdfpbd
    • -Xnodfpbd
    • -Xhysteresis

    These flags are not documented in any external documentation, either within OpenJ9 or within the IBM JDK. Searching on Google the only mention of these flags is from a 2006 CAS presentation. There is likely minimal to zero impact on end users. As part of this proposal we would keep the flags around but mark them for deprecation as they will not do anything when specified. OpenJ9 uses the OpenJDK BigDecimal and BigInteger implementations.

  2. The Java code supporting DFP is not open sourced. The Java source code is proprietary within the IBM JDK and not available within open source projects.
  3. During my tenure within the JIT compiler component (~8 years) I do not recall a single time anyone has ever debugged a DFP issue within the codebase. AFAIK no one within OpenJ9 is familiar with the code which is currently present in the JIT.
  4. There are no tests against the vast amount of DFP related code within the codebase, so even making changes in the area is very risky as we have no idea if changes made are functionally correct.
  5. DFP code within the JIT introduces quite a bit of logic on the control side which apparently forces BigDecimal methods to be compiled synchronously at count 0. If I had to guess this was likely for functional correctness purposes. None of this is documented.
  6. DFP code within the JIT relies on the presence of a Quad class which introduces conceptual integrity problems within our IL (makes our IL cyclic [reference needed]).
  7. DFP related opcodes within OpenJ9 account for roughly ~50% of the IL extended by OpenJ9, yet much of the IL is not touched by the optimizer at all, aside from some trivial simplification operations.
  8. There is some dead code associated with DFP and the DecimalData class from DAA. The Java methods related to the code are absent from the class, meaning this code is dead.
fjeremic commented 3 years ago

To give an idea of how invasive the code is and how much dead code we are talking about I've created draft PRs in eclipse/omr#5954 and eclipse/openj9#12534 which deprecate much of the code. Note that this was just a quick and dirty pass through the codebase. If we agree to the proposal I think it will have to be done piecemeal so we can properly review the code as DFP manages to entangle itself into areas which one may not have thought possible.

To summarize this is ~18000 lines of dead code which amounts to ~2.5% of the compiler component.

Here is a list of symbols which are likely to be deprecated (~500 of them):

isLongDouble
SUPPORT_DFP
generateS390DFPLongDoubleCompareAndBranchOps
getDFPTypeFromPrecision
getMaxExtendedDFPPrecision
getMaxShortDFPPrecision
getMaxLongDFPPrecision
isDFP
dfpToPackedCleanOp
dfpToZonedCleanOp
dfpToZonedSetSignOp
dfpToPackedSetSignOp
dfpToZonedOp
packedToDFPAbsOp
zonedToDFPAbsOp
dfpOpForBCDOp
insertExponentOp
setNegativeOpCode
shiftRightRoundedOpCode
lowerPackedShiftOrSetSignBelowDFPConv
df2zdEvaluator
zd2ddEvaluator
dfp2fpHelper
dfp2l64
dfp2lu64
l2dfp64
fixedToDFP
de2dxHelperAndSetRegister
dd2iEvaluator
pd2ddEvaluator
df2pdEvaluator
setupPackedDFPConversionGPRs
dfdivEvaluator
ddshlEvaluator
ddshrEvaluator
ddshrRoundedEvaluator
ddModifyPrecisionEvaluator
lu2dfp64
ddnegEvaluator
ddInsExpEvaluator
ddSetNegativeEvaluator
getPackedToDecimalDoubleFixedSize
getPackedToDecimalFloatFixedSize
supportsFastPackedDFPConversions
getPackedToDecimalLongDoubleFixedSize
getDecimalFloatToPackedFixedSize
getDecimalDoubleToPackedFixedSize
getDecimalLongDoubleToPackedFixedSize
SupportsZonedDFPConversions
getDFPPrecision
printDFPNodeInfo
isDFPModifyPrecision
TR_DisableArch11PackedToDFP
::DFP
DFPTestDataClass
setDFPPrecision
getRedundantDFPSetSignDescendant
TR_DisableZonedToDFPReduction
cancelDFPtoBCDtoBinaryConversion
df2pdSimplifier
deconstEvaluator
deloadEvaluator
destoreEvaluator
deRegLoadEvaluator
deRegStoreEvaluator
df2fEvaluator
f2dfEvaluator
i2ddEvaluator
l2ddEvaluator
lu2ddEvaluator
dd2lEvaluator
dd2luEvaluator
df2ddEvaluator
df2deEvaluator
dd2dfEvaluator
dd2deEvaluator
de2ddEvaluator
dfaddEvaluator
ddaddEvaluator
deaddEvaluator
dfsubEvaluator
ddsubEvaluator
desubEvaluator
dfmulEvaluator
ddmulEvaluator
demulEvaluator
ifddcmpeqEvaluator
ifddcmpneEvaluator
ifddcmpltEvaluator
ifddcmpgeEvaluator
ifddcmpgtEvaluator
ifddcmpleEvaluator
ifddcmpequEvaluator
ifddcmpneuEvaluator
ifddcmpltuEvaluator
ifddcmpgeuEvaluator
ifddcmpgtuEvaluator
ifddcmpleuEvaluator
ddcmpeqEvaluator
ddcmpneEvaluator
ddcmpltEvaluator
ddcmpgeEvaluator
ddcmpgtEvaluator
ddcmpleEvaluator
ddcmpequEvaluator
ddcmpneuEvaluator
ddcmpltuEvaluator
ddcmpgeuEvaluator
ddcmpgtuEvaluator
ddcmpleuEvaluator
dffloorEvaluator
ddfloorEvaluator
defloorEvaluator
ddcleanEvaluator
decleanEvaluator
de2iEvaluator
dfpSetSignSimplifier
deloadHelper
destoreHelper
DecimalFloat
DecimalDouble
DecimalLongDouble
dfconst
ddconst
deconst
dfload
ddload
deload
dfloadi
ddloadi
deloadi
dfstore
ddstore
destore
dfstorei
ddstorei
destorei
dfreturn
ddreturn
dereturn
dfcall
ddcall
decall
dfcalli
ddcalli
decalli
dfadd
ddadd
deadd
dfsub
ddsub
desub
dfmul
ddmul
demul
dfdiv
dddiv
dediv
dfrem
ddrem
derem
dfneg
ddneg
deneg
dfabs
ddabs
deabs
dfshl
dfshr
ddshl
ddshr
deshl
deshr
dfshrRounded
ddshrRounded
deshrRounded
dfSetNegative
ddSetNegative
deSetNegative
dfModifyPrecision
ddModifyPrecision
deModifyPrecision
i2df
iu2df
l2df
lu2df
f2df
d2df
dd2df
de2df
b2df
bu2df
s2df
su2df
df2i
df2iu
df2l
df2lu
df2f
df2d
df2dd
df2de
df2b
df2bu
df2s
df2c
i2dd
iu2dd
l2dd
lu2dd
f2dd
d2dd
de2dd
b2dd
bu2dd
s2dd
su2dd
dd2i
dd2iu
dd2l
dd2lu
dd2f
dd2d
dd2de
dd2b
dd2bu
dd2s
dd2c
i2de
iu2de
l2de
lu2de
f2de
d2de
b2de
bu2de
s2de
su2de
de2i
de2iu
de2l
de2lu
de2f
de2d
de2b
de2bu
de2s
de2c
ifdfcmpeq
ifdfcmpne
ifdfcmplt
ifdfcmpge
ifdfcmpgt
ifdfcmple
ifdfcmpequ
ifdfcmpneu
ifdfcmpltu
ifdfcmpgeu
ifdfcmpgtu
ifdfcmpleu
dfcmpeq
dfcmpne
dfcmplt
dfcmpge
dfcmpgt
dfcmple
dfcmpequ
dfcmpneu
dfcmpltu
dfcmpgeu
dfcmpgtu
dfcmpleu
ifddcmpeq
ifddcmpne
ifddcmplt
ifddcmpge
ifddcmpgt
ifddcmple
ifddcmpequ
ifddcmpneu
ifddcmpltu
ifddcmpgeu
ifddcmpgtu
ifddcmpleu
ddcmpeq
ddcmpne
ddcmplt
ddcmpge
ddcmpgt
ddcmple
ddcmpequ
ddcmpneu
ddcmpltu
ddcmpgeu
ddcmpgtu
ddcmpleu
ifdecmpeq
ifdecmpne
ifdecmplt
ifdecmpge
ifdecmpgt
ifdecmple
ifdecmpequ
ifdecmpneu
ifdecmpltu
ifdecmpgeu
ifdecmpgtu
ifdecmpleu
decmpeq
decmpne
decmplt
decmpge
decmpgt
decmple
decmpequ
decmpneu
decmpltu
decmpgeu
decmpgtu
decmpleu
dfRegLoad
ddRegLoad
deRegLoad
dfRegStore
ddRegStore
deRegStore
dfselect
ddselect
deselect
dfexp
ddexp
deexp
dfnint
ddnint
denint
dfsqrt
ddsqrt
desqrt
dfcos
ddcos
decos
dfsin
ddsin
desin
dftan
ddtan
detan
dfcosh
ddcosh
decosh
dfsinh
ddsinh
desinh
dftanh
ddtanh
detanh
dfacos
ddacos
deacos
dfasin
ddasin
deasin
dfatan
ddatan
deatan
dfatan2
ddatan2
deatan2
dflog
ddlog
delog
dffloor
ddfloor
defloor
dfceil
ddceil
deceil
dfmax
ddmax
demax
dfmin
ddmin
demin
dfInsExp
ddInsExp
deInsExp
ddclean
declean
zd2df
df2zd
zd2dd
dd2zd
zd2de
de2zd
zd2dfAbs
zd2ddAbs
zd2deAbs
df2zdSetSign
dd2zdSetSign
de2zdSetSign
df2zdClean
dd2zdClean
de2zdClean
pd2df
pd2dfAbs
df2pd
df2pdSetSign
df2pdClean
pd2dd
pd2ddAbs
dd2pd
dd2pdSetSign
dd2pdClean
pd2de
pd2deAbs
de2pd
de2pdSetSign
de2pdClean
genDFPGetHWAvailable
DFPGetHWAvailable
TR_DisableHysteresis
TR_DisableDFP
DFPFacilityAvailable
java_math_BigDecimal_DFPIntConstructor
java_math_BigDecimal_DFPLongConstructor
java_math_BigDecimal_DFPLongExpConstructor
java_math_BigDecimal_DFPAdd
java_math_BigDecimal_DFPSubtract
java_math_BigDecimal_DFPMultiply
java_math_BigDecimal_DFPDivide
java_math_BigDecimal_DFPScaledAdd
java_math_BigDecimal_DFPScaledSubtract
java_math_BigDecimal_DFPScaledMultiply
java_math_BigDecimal_DFPScaledDivide
java_math_BigDecimal_DFPRound
java_math_BigDecimal_DFPSetScale
java_math_BigDecimal_DFPCompareTo
java_math_BigDecimal_DFPSignificance
java_math_BigDecimal_DFPExponent
java_math_BigDecimal_DFPBCDDigits
java_math_BigDecimal_DFPUnscaledValue
inlineBigDecimal
inlineBigDecimalConstructor
inlineBigDecimalBinaryOp
inlineBigDecimalDivide
inlineBigDecimalRound
inlineBigDecimalUnaryOp
inlineBigDecimalCompareTo
inlineBigDecimalUnscaledValue
inlineBigDecimalSetScale
useDFPHardware
java_math_BigDecimal_DFPHWAvailable
java_math_BigDecimal_DFPUseDFP
java_math_BigDecimal_DFPPerformHysteresis
java_math_BigDecimal_DFPConvertPackedToDFP
java_math_BigDecimal_DFPConvertDFPToPacked
com_ibm_dataaccess_DecimalData_DFPUseDFP
com_ibm_dataaccess_DecimalData_DFPConvertPackedToDFP
com_ibm_dataaccess_DecimalData_DFPConvertDFPToPacked
com_ibm_dataaccess_DecimalData_createZeroBigDecimal
com_ibm_dataaccess_DecimalData_getlaside
com_ibm_dataaccess_DecimalData_setlaside
com_ibm_dataaccess_DecimalData_getflags
com_ibm_dataaccess_DecimalData_setflags
java_math_BigDecimal_DFPGetHWAvailable
isZonedToDFPAbs
isPackedToDFPAbs
isZonedOrPackedToDFPAbs
BD_EXT_FACILITY_AVAILABLE
BD_EXT_FACILITY_AVAILABLE_LEN
BD_EXT_USE_DFP
BD_EXT_USE_DFP_LEN
BD_DFP_HW_AVAILABLE_FLAG
BD_DFP_HW_AVAILABLE_FLAG_LEN
BD_DFP_HW_AVAILABLE_FLAG_SIG
BD_DFP_HW_AVAILABLE_FLAG_SIG_LEN
BD_DFP_HW_AVAILABLE_GET_STATIC_FIELD_ADDR_FLAG
removeUnnecessaryDFPClean
zd2dfSimplifier
df2zdSimplifier
df2zdSetSignSimplifier
pd2dfSimplifier
ifDFPCompareSimplifier
dfp2dfpSimplifier
df2pdSetSignSimplifier
int2dfpSimplifier
dfp2intSimplifier
dfpShiftLeftSimplifier
dfpShiftRightSimplifier
dfpModifyPrecisionSimplifier
dfpFloorSimplifier
dfpAddSimplifier
dfpMulSimplifier
dfpDivSimplifier
genStoreDFP
genClearFPSCRBits
genSetDFPRoundingMode
overlapDFPOperandAndPrecisionLoad
genLoadDFP
genRestoreFPCMaskBits
genClearFPCBits
genTestDataGroup
dsouzai commented 3 years ago

DFP support is disabled by default and has been for a long time.

There are no tests against the vast amount of DFP related code within the codebase

+1 to deprecate; if it's not enabled by default and not tested, it should be deleted.

0xdaryl commented 3 years ago

If this code is effectively dead (and has been for some time), not accessible to existing users, and we see no immediate future value in it then I'm in favour of removing it.

fjeremic commented 3 years ago

@gita-omr @zl-wang @joransiu @vijaysun-omr FYI if you have any concerns here.

joransiu commented 3 years ago

There is some dead code associated with DFP and the DecimalData class from DAA. The Java methods related to the code are absent from the class, meaning this code is dead.

@fjeremic : Just to confirm, you are referencing the lack of DFP scenarios in the convertBigDecimalToXX and convertXXToBigDecimal APIs in DataDecimal class -- hence, we should never be generating dfp related IL ops.

I somehow thought we attempted to exploit DFP in the past to accelerate some other DAA non-conversion operations (i.e. pd2dfp then dfp-to-String), but am not seeing it in the code now.

fjeremic commented 3 years ago

@fjeremic : Just to confirm, you are referencing the lack of DFP scenarios in the convertBigDecimalToXX and convertXXToBigDecimal APIs in DataDecimal class -- hence, we should never be generating dfp related IL ops.

I somehow thought we attempted to exploit DFP in the past to accelerate some other DAA non-conversion operations (i.e. pd2dfp then dfp-to-String), but am not seeing it in the code now.

That's right. If you look at the WIP PRs which give the example of the code we're talking about, here are the recognized methods:

      {x(TR::com_ibm_dataaccess_DecimalData_DFPFacilityAvailable,  "DFPFacilityAvailable",        "()Z")},
      {x(TR::com_ibm_dataaccess_DecimalData_DFPUseDFP,             "DFPUseDFP",             "()Z")},
      {x(TR::com_ibm_dataaccess_DecimalData_DFPConvertPackedToDFP, "DFPConvertPackedToDFP", "(Ljava/math/BigDecimal;JIZ)Z")},
      {x(TR::com_ibm_dataaccess_DecimalData_DFPConvertDFPToPacked, "DFPConvertDFPToPacked", "(JZ)J")},
      {x(TR::com_ibm_dataaccess_DecimalData_createZeroBigDecimal, "createZeroBigDecimal", "()Ljava/math/BigDecimal;")},
      {x(TR::com_ibm_dataaccess_DecimalData_getlaside, "getlaside", "(Ljava/math/BigDecimal;)J")},
      {x(TR::com_ibm_dataaccess_DecimalData_setlaside, "setlaside", "(Ljava/math/BigDecimal;J)V")},
      {x(TR::com_ibm_dataaccess_DecimalData_getflags, "getflags", "(Ljava/math/BigDecimal;)I")},
      {x(TR::com_ibm_dataaccess_DecimalData_setflags, "setflags", "(Ljava/math/BigDecimal;I)V")},
      {x(TR::com_ibm_dataaccess_DecimalData_slowSignedPackedToBigDecimal, "slowSignedPackedToBigDecimal", "([BIIIZ)Ljava/math/BigDecimal;")},
      {x(TR::com_ibm_dataaccess_DecimalData_slowBigDecimalToSignedPacked, "slowBigDecimalToSignedPacked", "(Ljava/math/BigDecimal;[BIIZ)V")},

Only a few of these methods exist in the DecimalData class which can be found here: https://github.com/eclipse/openj9/blob/b5cb6669757c51d8bf5eb9882ee396f4a4533502/jcl/src/openj9.dataaccess/share/classes/com/ibm/dataaccess/DecimalData.java

Looking at the list of those that do exist they're private methods and I don't see any uses of them within the class. So it is all dead code.

joransiu commented 3 years ago

Thanks @fjeremic. No further concerns from me, and agree this is good cleanup.

vijaysun-omr commented 3 years ago

I support this code cleanup.