Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit c10b304

Browse files
committed
Fix NEG decomposition to mark instructions that set and use flags
Fixes bad codegen in System.Math.Sign(long): ``` return unchecked((int)(value >> 63 | (long)((ulong)-value >> 63))); ``` where the flag-setting low part of the decomposed NEG was removed as dead by the constant right shift with constant >= 32, even though the flag setting is needed by the upper part. Fixes the MathSign5 failure of #14860.
1 parent 945de2f commit c10b304

File tree

1 file changed

+18
-4
lines changed

1 file changed

+18
-4
lines changed

src/jit/decomposelongs.cpp

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -952,13 +952,27 @@ GenTree* DecomposeLongs::DecomposeNeg(LIR::Use& use)
952952
loResult->gtOp.gtOp1 = loOp1;
953953

954954
GenTree* zero = m_compiler->gtNewZeroConNode(TYP_INT);
955+
955956
#if defined(_TARGET_X86_)
957+
956958
GenTree* hiAdjust = m_compiler->gtNewOperNode(GT_ADD_HI, TYP_INT, hiOp1, zero);
957959
GenTree* hiResult = m_compiler->gtNewOperNode(GT_NEG, TYP_INT, hiAdjust);
958960
Range().InsertAfter(loResult, zero, hiAdjust, hiResult);
961+
962+
loResult->gtFlags |= GTF_SET_FLAGS;
963+
hiAdjust->gtFlags |= GTF_USE_FLAGS;
964+
959965
#elif defined(_TARGET_ARM_)
966+
967+
// We tend to use "movs" to load zero to a register, and that sets the flags, so put the
968+
// zero before the loResult, which is setting the flags needed by GT_SUB_HI.
960969
GenTree* hiResult = m_compiler->gtNewOperNode(GT_SUB_HI, TYP_INT, zero, hiOp1);
961-
Range().InsertAfter(loResult, zero, hiResult);
970+
Range().InsertBefore(loResult, zero);
971+
Range().InsertAfter(loResult, hiResult);
972+
973+
loResult->gtFlags |= GTF_SET_FLAGS;
974+
hiResult->gtFlags |= GTF_USE_FLAGS;
975+
962976
#endif
963977

964978
return FinalizeDecomposition(use, loResult, hiResult, hiResult);
@@ -1198,7 +1212,7 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
11981212
//
11991213
// TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything
12001214
// that feeds the lo operand while there are no side effects)
1201-
if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
1215+
if ((loOp1->gtFlags & (GTF_ALL_EFFECT | GTF_SET_FLAGS)) == 0)
12021216
{
12031217
Range().Remove(loOp1, true);
12041218
}
@@ -1258,7 +1272,7 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
12581272
//
12591273
// TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
12601274
// feeds the lo operand while there are no side effects)
1261-
if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
1275+
if ((loOp1->gtFlags & (GTF_ALL_EFFECT | GTF_SET_FLAGS)) == 0)
12621276
{
12631277
Range().Remove(loOp1, true);
12641278
}
@@ -1356,7 +1370,7 @@ GenTree* DecomposeLongs::DecomposeShift(LIR::Use& use)
13561370
//
13571371
// TODO-CQ: we could go perform this removal transitively (i.e. iteratively remove everything that
13581372
// feeds the lo operand while there are no side effects)
1359-
if ((loOp1->gtFlags & GTF_ALL_EFFECT) == 0)
1373+
if ((loOp1->gtFlags & (GTF_ALL_EFFECT | GTF_SET_FLAGS)) == 0)
13601374
{
13611375
Range().Remove(loOp1, true);
13621376
}

0 commit comments

Comments
 (0)