Skip to content

Commit f12d847

Browse files
MikeHolmanMikeHolman
committed
fix issue where hoisted bound checks incorrectly calculated range of indexes
1 parent dd5b2e7 commit f12d847

File tree

4 files changed

+60
-11
lines changed

4 files changed

+60
-11
lines changed

‎lib/Backend/GlobOpt.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -709,7 +709,7 @@ class GlobOpt
709709
void DetermineLoopCount(Loop *const loop);
710710
void GenerateLoopCount(Loop *const loop, LoopCount *const loopCount);
711711
void GenerateLoopCountPlusOne(Loop *const loop, LoopCount *const loopCount);
712-
void GenerateSecondaryInductionVariableBound(Loop *const loop, StackSym *const inductionVariableSym, const LoopCount *const loopCount, const int maxMagnitudeChange, StackSym *const boundSym);
712+
void GenerateSecondaryInductionVariableBound(Loop *const loop, StackSym *const inductionVariableSym, LoopCount *const loopCount, const int maxMagnitudeChange, const bool needsMagnitudeAdjustment, StackSym *const boundSym);
713713

714714
private:
715715
void DetermineArrayBoundCheckHoistability(bool needLowerBoundCheck, bool needUpperBoundCheck, ArrayLowerBoundCheckHoistInfo &lowerHoistInfo, ArrayUpperBoundCheckHoistInfo &upperHoistInfo, const bool isJsArray, StackSym *const indexSym, Value *const indexValue, const IntConstantBounds &indexConstantBounds, StackSym *const headSegmentLengthSym, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, Loop *const headSegmentLengthInvariantLoop, bool &failedToUpdateCompatibleLowerBoundCheck, bool &failedToUpdateCompatibleUpperBoundCheck);

‎lib/Backend/GlobOptArrays.cpp

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -902,30 +902,37 @@ void GlobOpt::ArraySrcOpt::DoLowerBoundCheck()
902902
Assert(hoistInfo.Loop()->bailOutInfo);
903903
globOpt->EnsureBailTarget(hoistInfo.Loop());
904904

905+
bool needsMagnitudeAdjustment = false;
905906
if (hoistInfo.LoopCount())
906907
{
907908
// Generate the loop count and loop count based bound that will be used for the bound check
908909
if (!hoistInfo.LoopCount()->HasBeenGenerated())
909910
{
910911
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
911912
}
913+
needsMagnitudeAdjustment = (hoistInfo.MaxMagnitudeChange() > 0)
914+
? (hoistInfo.IndexOffset() < hoistInfo.MaxMagnitudeChange())
915+
: (hoistInfo.IndexOffset() > hoistInfo.MaxMagnitudeChange());
916+
912917
globOpt->GenerateSecondaryInductionVariableBound(
913918
hoistInfo.Loop(),
914919
indexVarSym->GetInt32EquivSym(nullptr),
915920
hoistInfo.LoopCount(),
916921
hoistInfo.MaxMagnitudeChange(),
922+
needsMagnitudeAdjustment,
917923
hoistInfo.IndexSym());
918924
}
919925

920926
IR::Opnd* lowerBound = IR::IntConstOpnd::New(0, TyInt32, instr->m_func, true);
921927
IR::Opnd* upperBound = IR::RegOpnd::New(indexIntSym, TyInt32, instr->m_func);
928+
int offset = needsMagnitudeAdjustment ? (hoistInfo.IndexOffset() - hoistInfo.Offset()) : hoistInfo.Offset();
922929
upperBound->SetIsJITOptimizedReg(true);
923930

924931
// 0 <= indexSym + offset (src1 <= src2 + dst)
925932
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
926933
lowerBound,
927934
upperBound,
928-
hoistInfo.Offset(),
935+
offset,
929936
hoistInfo.IsLoopCountBasedBound()
930937
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
931938
: IR::BailOutOnFailedHoistedBoundCheck,
@@ -1148,18 +1155,23 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
11481155
Assert(hoistInfo.Loop()->bailOutInfo);
11491156
globOpt->EnsureBailTarget(hoistInfo.Loop());
11501157

1158+
bool needsMagnitudeAdjustment = false;
11511159
if (hoistInfo.LoopCount())
11521160
{
11531161
// Generate the loop count and loop count based bound that will be used for the bound check
11541162
if (!hoistInfo.LoopCount()->HasBeenGenerated())
11551163
{
11561164
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
11571165
}
1166+
needsMagnitudeAdjustment = (hoistInfo.MaxMagnitudeChange() > 0)
1167+
? (hoistInfo.IndexOffset() < hoistInfo.MaxMagnitudeChange())
1168+
: (hoistInfo.IndexOffset() > hoistInfo.MaxMagnitudeChange());
11581169
globOpt->GenerateSecondaryInductionVariableBound(
11591170
hoistInfo.Loop(),
11601171
indexVarSym->GetInt32EquivSym(nullptr),
11611172
hoistInfo.LoopCount(),
11621173
hoistInfo.MaxMagnitudeChange(),
1174+
needsMagnitudeAdjustment,
11631175
hoistInfo.IndexSym());
11641176
}
11651177

@@ -1174,11 +1186,13 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
11741186
IR::Opnd* upperBound = IR::RegOpnd::New(headSegmentLengthSym, headSegmentLengthSym->GetType(), instr->m_func);
11751187
upperBound->SetIsJITOptimizedReg(true);
11761188

1189+
int offset = needsMagnitudeAdjustment ? (hoistInfo.IndexOffset() + hoistInfo.Offset()) : hoistInfo.Offset();
1190+
11771191
// indexSym <= headSegmentLength + offset (src1 <= src2 + dst)
11781192
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
11791193
lowerBound,
11801194
upperBound,
1181-
hoistInfo.Offset(),
1195+
offset,
11821196
hoistInfo.IsLoopCountBasedBound()
11831197
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
11841198
: IR::BailOutOnFailedHoistedBoundCheck,
@@ -1197,7 +1211,7 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
11971211
landingPad->GetBlockNum(),
11981212
hoistInfo.IndexSym()->m_id,
11991213
headSegmentLengthSym->m_id,
1200-
hoistInfo.Offset());
1214+
offset);
12011215
}
12021216
else
12031217
{
@@ -1209,7 +1223,7 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
12091223
landingPad->GetBlockNum(),
12101224
hoistInfo.IndexConstantBounds().LowerBound(),
12111225
headSegmentLengthSym->m_id,
1212-
hoistInfo.Offset());
1226+
offset);
12131227
}
12141228

12151229
TESTTRACE_PHASE_INSTR(

‎lib/Backend/GlobOptIntBounds.cpp

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(
7878
void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(
7979
::Loop *const loop,
8080
StackSym *const indexSym,
81+
const int indexOffset,
8182
const int offset,
8283
Value *const indexValue,
8384
const IntConstantBounds &indexConstantBounds,
@@ -91,6 +92,7 @@ void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(
9192

9293
this->loop = loop;
9394
this->indexSym = indexSym;
95+
this->indexOffset = indexOffset;
9496
this->offset = offset;
9597
this->indexValueNumber = indexValue->GetValueNumber();
9698
this->indexValue = indexValue;
@@ -142,6 +144,7 @@ void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
142144
void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
143145
::Loop *const loop,
144146
StackSym *const indexSym,
147+
const int indexOffset,
145148
const int offset,
146149
Value *const indexValue,
147150
const IntConstantBounds &indexConstantBounds,
@@ -151,7 +154,7 @@ void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
151154
{
152155
Assert(headSegmentLengthValue);
153156

154-
SetLoop(loop, indexSym, offset, indexValue, indexConstantBounds, isLoopCountBasedBound);
157+
SetLoop(loop, indexSym, indexOffset, offset, indexValue, indexConstantBounds, isLoopCountBasedBound);
155158
this->headSegmentLengthValue = headSegmentLengthValue;
156159
this->headSegmentLengthConstantBounds = headSegmentLengthConstantBounds;
157160
}
@@ -1831,8 +1834,9 @@ void GlobOpt::GenerateLoopCountPlusOne(Loop *const loop, LoopCount *const loopCo
18311834
void GlobOpt::GenerateSecondaryInductionVariableBound(
18321835
Loop *const loop,
18331836
StackSym *const inductionVariableSym,
1834-
const LoopCount *const loopCount,
1837+
LoopCount *const loopCount,
18351838
const int maxMagnitudeChange,
1839+
const bool needsMagnitudeAdjustment,
18361840
StackSym *const boundSym)
18371841
{
18381842
Assert(loop);
@@ -1857,18 +1861,33 @@ void GlobOpt::GenerateSecondaryInductionVariableBound(
18571861
Assert(insertBeforeInstr);
18581862
Func *const func = bailOutInfo->bailOutFunc;
18591863

1864+
StackSym* loopCountSym = nullptr;
1865+
1866+
// If indexOffset < maxMagnitudeChange, we need to account for the difference between them in the bound check
1867+
// i.e. BoundCheck: inductionVariable + loopCountMinusOne * maxMagnitudeChange + maxMagnitudeChange - indexOffset <= length - offset
1868+
// Since the BoundCheck instruction already deals with offset, we can simplify this to
1869+
// BoundCheck: inductionVariable + loopCount * maxMagnitudeChange <= length + indexOffset - offset
1870+
if (needsMagnitudeAdjustment)
1871+
{
1872+
GenerateLoopCountPlusOne(loop, loopCount);
1873+
loopCountSym = loopCount->LoopCountSym();
1874+
}
1875+
else
1876+
{
1877+
loopCountSym = loopCount->LoopCountMinusOneSym();
1878+
}
18601879
// intermediateValue = loopCount * maxMagnitudeChange
18611880
StackSym *intermediateValueSym;
18621881
if(maxMagnitudeChange == 1 || maxMagnitudeChange == -1)
18631882
{
1864-
intermediateValueSym = loopCount->LoopCountMinusOneSym();
1883+
intermediateValueSym = loopCountSym;
18651884
}
18661885
else
18671886
{
18681887
IR::BailOutInstr *const instr = IR::BailOutInstr::New(Js::OpCode::Mul_I4, bailOutKind, bailOutInfo, func);
18691888

18701889
instr->SetSrc1(
1871-
IR::RegOpnd::New(loopCount->LoopCountMinusOneSym(), loopCount->LoopCountMinusOneSym()->GetType(), func));
1890+
IR::RegOpnd::New(loopCountSym, loopCountSym->GetType(), func));
18721891
instr->GetSrc1()->SetIsJITOptimizedReg(true);
18731892

18741893
instr->SetSrc2(IR::IntConstOpnd::New(maxMagnitudeChange, TyInt32, func, true));
@@ -2418,6 +2437,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
24182437
loop,
24192438
indexSym,
24202439
lowerOffset,
2440+
lowerOffset,
24212441
landingPadIndexValue,
24222442
landingPadIndexConstantBounds);
24232443
}
@@ -2469,11 +2489,13 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
24692489
// Normalize the offset such that:
24702490
// boundBase <= headSegmentLength + offset
24712491
// Where (offset = -1 - boundOffset), and -1 is to simulate < instead of <=.
2492+
int indexOffset = upperOffset;
24722493
upperOffset = -1 - upperOffset;
24732494

24742495
upperHoistInfo.SetLoop(
24752496
loop,
24762497
indexSym,
2498+
indexOffset,
24772499
upperOffset,
24782500
landingPadIndexValue,
24792501
landingPadIndexConstantBounds,
@@ -2619,6 +2641,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
26192641
loop,
26202642
indexBoundBaseSym,
26212643
offset,
2644+
offset,
26222645
landingPadIndexBoundBaseValue,
26232646
landingPadIndexBoundBaseConstantBounds);
26242647
break;
@@ -2643,11 +2666,13 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
26432666
// Normalize the offset such that:
26442667
// boundBase <= headSegmentLength + offset
26452668
// Where (offset = -1 - boundOffset), and -1 is to simulate < instead of <=.
2669+
int indexOffset = offset;
26462670
offset = -1 - offset;
26472671

26482672
upperHoistInfo.SetLoop(
26492673
loop,
26502674
indexBoundBaseSym,
2675+
indexOffset,
26512676
offset,
26522677
landingPadIndexBoundBaseValue,
26532678
landingPadIndexBoundBaseConstantBounds,
@@ -3139,6 +3164,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
31393164
lowerHoistInfo.SetLoop(
31403165
currentLoop,
31413166
indexLoopCountBasedBoundBaseSym,
3167+
indexOffset,
31423168
offset,
31433169
indexLoopCountBasedBoundBaseValue,
31443170
indexLoopCountBasedBoundBaseConstantBounds,
@@ -3153,6 +3179,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
31533179
upperHoistInfo.SetLoop(
31543180
currentLoop,
31553181
indexLoopCountBasedBoundBaseSym,
3182+
indexOffset,
31563183
offset,
31573184
indexLoopCountBasedBoundBaseValue,
31583185
indexLoopCountBasedBoundBaseConstantBounds,

‎lib/Backend/GlobOptIntBounds.h

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo
240240

241241
// Info populated for a compatible bound check and for hoisting out of loop
242242
StackSym *indexSym;
243+
int indexOffset;
243244
int offset;
244245
ValueNumber indexValueNumber;
245246

@@ -287,6 +288,12 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo
287288
return offset;
288289
}
289290

291+
int32 IndexOffset() const
292+
{
293+
Assert(HasAnyInfo());
294+
return indexOffset;
295+
}
296+
290297
void UpdateOffset(int newOffset)
291298
{
292299
Assert(HasAnyInfo());
@@ -334,7 +341,7 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo
334341
public:
335342
void SetCompatibleBoundCheck(BasicBlock *const compatibleBoundCheckBlock, StackSym *const indexSym, const int offset, const ValueNumber indexValueNumber);
336343
void SetLoop(::Loop *const loop, const int indexConstantValue, const bool isLoopCountBasedBound = false);
337-
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, const bool isLoopCountBasedBound = false);
344+
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int indexOffset, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, const bool isLoopCountBasedBound = false);
338345
void SetLoopCount(::LoopCount *const loopCount, const int maxMagnitudeChange);
339346
};
340347

@@ -353,6 +360,7 @@ class GlobOpt::ArrayUpperBoundCheckHoistInfo : protected ArrayLowerBoundCheckHoi
353360
using Base::CompatibleBoundCheckBlock;
354361
using Base::Loop;
355362
using Base::IndexSym;
363+
using Base::IndexOffset;
356364
using Base::Offset;
357365
using Base::UpdateOffset;
358366
using Base::IndexValueNumber;
@@ -385,5 +393,5 @@ class GlobOpt::ArrayUpperBoundCheckHoistInfo : protected ArrayLowerBoundCheckHoi
385393
public:
386394
void SetCompatibleBoundCheck(BasicBlock *const compatibleBoundCheckBlock, const int indexConstantValue);
387395
void SetLoop(::Loop *const loop, const int indexConstantValue, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
388-
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
396+
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int indexOffset, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
389397
};

0 commit comments

Comments
 (0)