Skip to content

Commit e9d6a3e

Browse files
MikeHolmananeeshdk
MikeHolman
authored and
aneeshdk
committed
remove unsafe bound check elimination
When deciding if we can eliminate a bound check we check if the max value for the index is less than the min value for the length. If this is true, we can remove the bound check. In the code we were testing if indexUpperBound <= lengthLowerBound + (-1 - indexLowerBound). If the index lower bound is less than 0 (e.g. if it is INT_MIN because we don't know the range), we may incorrectly eliminate bound checks. This was essentially never kicking in, so I removed the condition altogether.
1 parent f00612b commit e9d6a3e

File tree

3 files changed

+99
-101
lines changed

3 files changed

+99
-101
lines changed

‎lib/Backend/GlobOptArrays.cpp

Lines changed: 78 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -897,19 +897,8 @@ void GlobOpt::ArraySrcOpt::DoLowerBoundCheck()
897897
Assert(!indexIntSym || indexIntSym->GetType() == TyInt32 || indexIntSym->GetType() == TyUint32);
898898
}
899899

900-
// The info in the landing pad may be better than the info in the current block due to changes made to
901-
// the index sym inside the loop. Check if the bound check we intend to hoist is unnecessary in the
902-
// landing pad.
903-
if (!ValueInfo::IsLessThanOrEqualTo(
904-
nullptr,
905-
0,
906-
0,
907-
hoistInfo.IndexValue(),
908-
hoistInfo.IndexConstantBounds().LowerBound(),
909-
hoistInfo.IndexConstantBounds().UpperBound(),
910-
hoistInfo.Offset()))
900+
if (hoistInfo.IndexSym())
911901
{
912-
Assert(hoistInfo.IndexSym());
913902
Assert(hoistInfo.Loop()->bailOutInfo);
914903
globOpt->EnsureBailTarget(hoistInfo.Loop());
915904

@@ -1156,106 +1145,94 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
11561145
Assert(!indexIntSym || indexIntSym->GetType() == TyInt32 || indexIntSym->GetType() == TyUint32);
11571146
}
11581147

1159-
// The info in the landing pad may be better than the info in the current block due to changes made to the
1160-
// index sym inside the loop. Check if the bound check we intend to hoist is unnecessary in the landing pad.
1161-
if (!ValueInfo::IsLessThanOrEqualTo(
1162-
hoistInfo.IndexValue(),
1163-
hoistInfo.IndexConstantBounds().LowerBound(),
1164-
hoistInfo.IndexConstantBounds().UpperBound(),
1165-
hoistInfo.HeadSegmentLengthValue(),
1166-
hoistInfo.HeadSegmentLengthConstantBounds().LowerBound(),
1167-
hoistInfo.HeadSegmentLengthConstantBounds().UpperBound(),
1168-
hoistInfo.Offset()))
1169-
{
1170-
Assert(hoistInfo.Loop()->bailOutInfo);
1171-
globOpt->EnsureBailTarget(hoistInfo.Loop());
1148+
Assert(hoistInfo.Loop()->bailOutInfo);
1149+
globOpt->EnsureBailTarget(hoistInfo.Loop());
11721150

1173-
if (hoistInfo.LoopCount())
1151+
if (hoistInfo.LoopCount())
1152+
{
1153+
// Generate the loop count and loop count based bound that will be used for the bound check
1154+
if (!hoistInfo.LoopCount()->HasBeenGenerated())
11741155
{
1175-
// Generate the loop count and loop count based bound that will be used for the bound check
1176-
if (!hoistInfo.LoopCount()->HasBeenGenerated())
1177-
{
1178-
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
1179-
}
1180-
globOpt->GenerateSecondaryInductionVariableBound(
1181-
hoistInfo.Loop(),
1182-
indexVarSym->GetInt32EquivSym(nullptr),
1183-
hoistInfo.LoopCount(),
1184-
hoistInfo.MaxMagnitudeChange(),
1185-
hoistInfo.IndexSym());
1156+
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
11861157
}
1158+
globOpt->GenerateSecondaryInductionVariableBound(
1159+
hoistInfo.Loop(),
1160+
indexVarSym->GetInt32EquivSym(nullptr),
1161+
hoistInfo.LoopCount(),
1162+
hoistInfo.MaxMagnitudeChange(),
1163+
hoistInfo.IndexSym());
1164+
}
11871165

1188-
IR::Opnd* lowerBound = indexIntSym
1189-
? static_cast<IR::Opnd *>(IR::RegOpnd::New(indexIntSym, TyInt32, instr->m_func))
1190-
: IR::IntConstOpnd::New(
1191-
hoistInfo.IndexConstantBounds().LowerBound(),
1192-
TyInt32,
1193-
instr->m_func);
1194-
1195-
lowerBound->SetIsJITOptimizedReg(true);
1196-
IR::Opnd* upperBound = IR::RegOpnd::New(headSegmentLengthSym, headSegmentLengthSym->GetType(), instr->m_func);
1197-
upperBound->SetIsJITOptimizedReg(true);
1166+
IR::Opnd* lowerBound = indexIntSym
1167+
? static_cast<IR::Opnd *>(IR::RegOpnd::New(indexIntSym, TyInt32, instr->m_func))
1168+
: IR::IntConstOpnd::New(
1169+
hoistInfo.IndexConstantBounds().LowerBound(),
1170+
TyInt32,
1171+
instr->m_func);
11981172

1199-
// indexSym <= headSegmentLength + offset (src1 <= src2 + dst)
1200-
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
1201-
lowerBound,
1202-
upperBound,
1203-
hoistInfo.Offset(),
1204-
hoistInfo.IsLoopCountBasedBound()
1205-
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
1206-
: IR::BailOutOnFailedHoistedBoundCheck,
1207-
hoistInfo.Loop()->bailOutInfo,
1208-
hoistInfo.Loop()->bailOutInfo->bailOutFunc);
1173+
lowerBound->SetIsJITOptimizedReg(true);
1174+
IR::Opnd* upperBound = IR::RegOpnd::New(headSegmentLengthSym, headSegmentLengthSym->GetType(), instr->m_func);
1175+
upperBound->SetIsJITOptimizedReg(true);
12091176

1210-
InsertInstrInLandingPad(boundCheck, hoistInfo.Loop());
1177+
// indexSym <= headSegmentLength + offset (src1 <= src2 + dst)
1178+
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
1179+
lowerBound,
1180+
upperBound,
1181+
hoistInfo.Offset(),
1182+
hoistInfo.IsLoopCountBasedBound()
1183+
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
1184+
: IR::BailOutOnFailedHoistedBoundCheck,
1185+
hoistInfo.Loop()->bailOutInfo,
1186+
hoistInfo.Loop()->bailOutInfo->bailOutFunc);
12111187

1212-
if (indexIntSym)
1213-
{
1214-
TRACE_PHASE_INSTR(
1215-
Js::Phase::BoundCheckHoistPhase,
1216-
instr,
1217-
_u("Hoisting array upper bound check out of loop %u to landing pad block %u, as (s%u <= s%u + %d)\n"),
1218-
hoistInfo.Loop()->GetLoopNumber(),
1219-
landingPad->GetBlockNum(),
1220-
hoistInfo.IndexSym()->m_id,
1221-
headSegmentLengthSym->m_id,
1222-
hoistInfo.Offset());
1223-
}
1224-
else
1225-
{
1226-
TRACE_PHASE_INSTR(
1227-
Js::Phase::BoundCheckHoistPhase,
1228-
instr,
1229-
_u("Hoisting array upper bound check out of loop %u to landing pad block %u, as (%d <= s%u + %d)\n"),
1230-
hoistInfo.Loop()->GetLoopNumber(),
1231-
landingPad->GetBlockNum(),
1232-
hoistInfo.IndexConstantBounds().LowerBound(),
1233-
headSegmentLengthSym->m_id,
1234-
hoistInfo.Offset());
1235-
}
1188+
InsertInstrInLandingPad(boundCheck, hoistInfo.Loop());
12361189

1237-
TESTTRACE_PHASE_INSTR(
1190+
if (indexIntSym)
1191+
{
1192+
TRACE_PHASE_INSTR(
1193+
Js::Phase::BoundCheckHoistPhase,
1194+
instr,
1195+
_u("Hoisting array upper bound check out of loop %u to landing pad block %u, as (s%u <= s%u + %d)\n"),
1196+
hoistInfo.Loop()->GetLoopNumber(),
1197+
landingPad->GetBlockNum(),
1198+
hoistInfo.IndexSym()->m_id,
1199+
headSegmentLengthSym->m_id,
1200+
hoistInfo.Offset());
1201+
}
1202+
else
1203+
{
1204+
TRACE_PHASE_INSTR(
12381205
Js::Phase::BoundCheckHoistPhase,
12391206
instr,
1240-
_u("Hoisting array upper bound check out of loop\n"));
1207+
_u("Hoisting array upper bound check out of loop %u to landing pad block %u, as (%d <= s%u + %d)\n"),
1208+
hoistInfo.Loop()->GetLoopNumber(),
1209+
landingPad->GetBlockNum(),
1210+
hoistInfo.IndexConstantBounds().LowerBound(),
1211+
headSegmentLengthSym->m_id,
1212+
hoistInfo.Offset());
1213+
}
12411214

1242-
// Record the bound check instruction as available
1243-
const IntBoundCheck boundCheckInfo(
1244-
hoistInfo.IndexValue() ? hoistInfo.IndexValueNumber() : ZeroValueNumber,
1245-
hoistInfo.HeadSegmentLengthValue()->GetValueNumber(),
1246-
boundCheck,
1247-
landingPad);
1248-
{
1249-
const bool added = globOpt->CurrentBlockData()->availableIntBoundChecks->AddNew(boundCheckInfo) >= 0;
1250-
Assert(added || failedToUpdateCompatibleUpperBoundCheck);
1251-
}
1252-
for (InvariantBlockBackwardIterator it(globOpt, globOpt->currentBlock, landingPad, nullptr);
1253-
it.IsValid();
1254-
it.MoveNext())
1255-
{
1256-
const bool added = it.Block()->globOptData.availableIntBoundChecks->AddNew(boundCheckInfo) >= 0;
1257-
Assert(added || failedToUpdateCompatibleUpperBoundCheck);
1258-
}
1215+
TESTTRACE_PHASE_INSTR(
1216+
Js::Phase::BoundCheckHoistPhase,
1217+
instr,
1218+
_u("Hoisting array upper bound check out of loop\n"));
1219+
1220+
// Record the bound check instruction as available
1221+
const IntBoundCheck boundCheckInfo(
1222+
hoistInfo.IndexValue() ? hoistInfo.IndexValueNumber() : ZeroValueNumber,
1223+
hoistInfo.HeadSegmentLengthValue()->GetValueNumber(),
1224+
boundCheck,
1225+
landingPad);
1226+
{
1227+
const bool added = globOpt->CurrentBlockData()->availableIntBoundChecks->AddNew(boundCheckInfo) >= 0;
1228+
Assert(added || failedToUpdateCompatibleUpperBoundCheck);
1229+
}
1230+
for (InvariantBlockBackwardIterator it(globOpt, globOpt->currentBlock, landingPad, nullptr);
1231+
it.IsValid();
1232+
it.MoveNext())
1233+
{
1234+
const bool added = it.Block()->globOptData.availableIntBoundChecks->AddNew(boundCheckInfo) >= 0;
1235+
Assert(added || failedToUpdateCompatibleUpperBoundCheck);
12591236
}
12601237
}
12611238

‎test/Optimizer/bcebug.js

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
//-------------------------------------------------------------------------------------------------------
2+
// Copyright (C) Microsoft. All rights reserved.
3+
// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
4+
//-------------------------------------------------------------------------------------------------------
5+
6+
let arr = new Uint32Array(10);
7+
for (let i = 0; i < 11; i++) {
8+
for (let j = 0; j < 1; j++) {
9+
i--;
10+
i++;
11+
}
12+
arr[i] = 0x1234;
13+
}
14+
15+
print("Pass")

‎test/Optimizer/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1556,6 +1556,12 @@
15561556
<tags>exclude_dynapogo,exclude_nonative</tags>
15571557
</default>
15581558
</test>
1559+
<test>
1560+
<default>
1561+
<files>bcebug.js</files>
1562+
<compile-flags>-mic:1 -off:simplejit -bgjit- -lic:1</compile-flags>
1563+
</default>
1564+
</test>
15591565
<test>
15601566
<default>
15611567
<files>rembug.js</files>

0 commit comments

Comments
 (0)