Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JIT] ARM64 - Overflow check optimizations for division #82924

Merged
merged 29 commits into from
Mar 10, 2023
Merged

[JIT] ARM64 - Overflow check optimizations for division #82924

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
983523c
Do not emit overflow check for div if we know first operand is a cons…
TIHan Mar 3, 2023
81d7666
Remove redundant branch
TIHan Mar 3, 2023
0017311
Update codegenarm64.cpp
TIHan Mar 3, 2023
844ceff
Try to use subs, zr, dividendReg, 1 to determine overflow in division
TIHan Mar 3, 2023
28da6ca
Merge remote-tracking branch 'upstream/main' into redundant-overflow-…
TIHan Mar 6, 2023
1cb5b75
Cleanup
TIHan Mar 6, 2023
96bdde8
Merge remote-tracking branch 'upstream/main' into redundant-overflow-…
TIHan Mar 6, 2023
ffae5be
Use IsNeverNegative to help not emit overflow checks
TIHan Mar 6, 2023
1b90874
Fix check
TIHan Mar 6, 2023
07b9e6f
Added helper functions
TIHan Mar 6, 2023
0c19463
Use main node instead of op1 for type check
TIHan Mar 6, 2023
91e657b
Add 64-bit check
TIHan Mar 6, 2023
3ff0e20
Use cmp instead of subs since it is an alias
TIHan Mar 7, 2023
1f63cb2
Handle 32-bit arch division for LONG.
TIHan Mar 7, 2023
99e972a
Additional checks
TIHan Mar 7, 2023
090e378
More cleanup
TIHan Mar 7, 2023
4223d69
More cleanup
TIHan Mar 7, 2023
1e43e5e
Do not find exception blocks
TIHan Mar 7, 2023
dac7f80
Try using flags
TIHan Mar 7, 2023
e39057e
Try using flags
TIHan Mar 7, 2023
07b80e0
More checks, more comments
TIHan Mar 7, 2023
72d4713
Update src/coreclr/jit/gentree.cpp
TIHan Mar 8, 2023
84bb667
Flip meaning of check flags. Rename OperExceptions to GetExceptionSet…
TIHan Mar 8, 2023
722b820
Use GetExceptionSetFlags in arm64 codegen
TIHan Mar 8, 2023
6b8fc40
Revert name
TIHan Mar 8, 2023
b164f0b
Use GTF_DIV_MOD_NO_OVERFLOW_CHK in CanDivOrModPossiblyOverflow
TIHan Mar 8, 2023
f25f1ed
Fix build
TIHan Mar 8, 2023
0117364
Rename flags by removing '_CHK'
TIHan Mar 8, 2023
2431635
Remove break
TIHan Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/coreclr/jit/codegenarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3507,11 +3507,10 @@ void CodeGen::genCodeForDivMod(GenTreeOp* tree)
regNumber dividendReg = dividendOp->GetRegNum();
// At this point the divisor is known to be -1
//
// Issue the 'adds zr, dividendReg, dividendReg' instruction
// this will set both the Z and V flags only when dividendReg is MinInt
// Issue the 'subs zr, dividendReg,1' instruction
// this will set the V flags only when dividendReg is MinInt
//
emit->emitIns_R_R_R(INS_adds, size, REG_ZR, dividendReg, dividendReg);
inst_JMP(EJ_ne, sdivLabel); // goto sdiv if the Z flag is clear
emit->emitIns_R_R_I(INS_subs, size, REG_ZR, dividendReg, 1);
genJumpToThrowHlpBlk(EJ_vs, SCK_ARITH_EXCPN); // if the V flags is set throw
// ArithmeticException

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/emitarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5698,7 +5698,7 @@ void emitter::emitIns_R_R_I(

if (setFlags) // Can't encode SP with setFlags
{
assert(isGeneralRegister(reg1));
assert(isGeneralRegisterOrZR(reg1));
TIHan marked this conversation as resolved.
Show resolved Hide resolved
assert(isGeneralRegister(reg2));
}
else
Expand Down
20 changes: 19 additions & 1 deletion src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9675,7 +9675,25 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
// We do not need to throw if the second operand is a non-(negative one) constant.
if (!op2->IsIntegralConst() || op2->IsIntegralConst(-1))
{
fgAddCodeRef(compCurBB, bbThrowIndex(compCurBB), SCK_OVERFLOW);
bool checkDividend = true;

// We do not need to throw if we know the first operand is a constant and not a min-value.
if (op1->IsIntegralConst())
TIHan marked this conversation as resolved.
Show resolved Hide resolved
{
if (tree->TypeIs(TYP_INT) && !op1->IsIntegralConst(INT32_MIN))
{
checkDividend = false;
}
else if (tree->TypeIs(TYP_LONG) && !op1->IsIntegralConst(INT64_MIN))
{
checkDividend = false;
}
}

if (checkDividend)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this still leaves overflow block alive if it's unused but presumably it's a different issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hhm, won't it be dead code eliminated?

Copy link
Contributor

@SingleAccretion SingleAccretion Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw helper blocks are marked as no-remove. This is because they aren't rooted in the normal flowgraph (the edges from the throwing nodes to the throw helper blocks are implicit).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hhm, won't it be dead code eliminated?

these blocks are special, they have BBF_DONT_REMOVE or something like that. We spawn them early in importer/morph and then tie to corresponding divisions if needed, it's too late to remove them in codegen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hopeful that this PR will resolve those cases as it's already happening today.

{
fgAddCodeRef(compCurBB, bbThrowIndex(compCurBB), SCK_OVERFLOW);
}
}

// We do not need to throw if the second operand is a non-zero constant.
Expand Down