Skip to content

Commit 5533dbe

Browse files
author
Kevin Smith
committed
[MERGE #6257 @zenparsing] Fix value propagation on loop back-edge with aggressive value transfers
Merge pull request #6257 from zenparsing:loop-prepass-bug When using aggressive value transfer in loop prepasses, the data flow analyzer can incorrectly determine that two syms always share a value on the backedge, when in fact their values can diverge on subsequent iterations of the loop. This change ensures that all syms assigned to within a loop are given unique value numbers when merging from the backedge. The performance gains from the AVTInPrepass optimization (specifically Octane/deltablue) are retained with this fix. Fixes #6252
2 parents 5c8eccb + 4014ca6 commit 5533dbe

File tree

5 files changed

+66
-15
lines changed

5 files changed

+66
-15
lines changed

‎lib/Backend/GlobOptBlockData.cpp

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -767,12 +767,23 @@ GlobOptBlockData::MergeValueMaps(
767767

768768
if (iter2.IsValid() && bucket.value->m_id == iter2.Data().value->m_id)
769769
{
770+
// Syms that are assigned to within the loop must have unique
771+
// value numbers in the loop header after merging; a single
772+
// prepass is not adequate to determine that sym values are
773+
// equivalent through all possible loop paths.
774+
bool forceUniqueValue =
775+
isLoopBackEdge &&
776+
!this->globOpt->IsLoopPrePass() &&
777+
loop &&
778+
loop->symsAssignedToInLoop->Test(bucket.value->m_id);
779+
770780
newValue =
771781
this->MergeValues(
772782
bucket.element,
773783
iter2.Data().element,
774784
iter2.Data().value,
775785
isLoopBackEdge,
786+
forceUniqueValue,
776787
symsRequiringCompensation,
777788
symsCreatedForMerge);
778789
}
@@ -847,6 +858,7 @@ GlobOptBlockData::MergeValues(
847858
Value *fromDataValue,
848859
Sym *fromDataSym,
849860
bool isLoopBackEdge,
861+
bool forceUniqueValue,
850862
BVSparse<JitArenaAllocator> *const symsRequiringCompensation,
851863
BVSparse<JitArenaAllocator> *const symsCreatedForMerge)
852864
{
@@ -879,22 +891,30 @@ GlobOptBlockData::MergeValues(
879891
return toDataValue;
880892
}
881893

882-
// There may be other syms in toData that haven't been merged yet, referring to the current toData value for this sym. If
883-
// the merge produced a new value info, don't corrupt the value info for the other sym by changing the same value. Instead,
884-
// create one value per source value number pair per merge and reuse that for new value infos.
885-
Value *newValue = this->globOpt->valuesCreatedForMerge->Lookup(sourceValueNumberPair, nullptr);
886-
if(newValue)
894+
Value *newValue = nullptr;
895+
if (forceUniqueValue)
887896
{
888-
Assert(sameValueNumber == (newValue->GetValueNumber() == toDataValue->GetValueNumber()));
889-
890-
// This is an exception where Value::SetValueInfo is called directly instead of GlobOpt::ChangeValueInfo, because we're
891-
// actually generating new value info through merges.
892-
newValue->SetValueInfo(newValueInfo);
897+
newValue = this->globOpt->NewValue(newValueInfo);
893898
}
894899
else
895900
{
896-
newValue = this->globOpt->NewValue(sameValueNumber ? sourceValueNumberPair.First() : this->globOpt->NewValueNumber(), newValueInfo);
897-
this->globOpt->valuesCreatedForMerge->Add(sourceValueNumberPair, newValue);
901+
// There may be other syms in toData that haven't been merged yet, referring to the current toData value for this sym. If
902+
// the merge produced a new value info, don't corrupt the value info for the other sym by changing the same value. Instead,
903+
// create one value per source value number pair per merge and reuse that for new value infos.
904+
newValue = this->globOpt->valuesCreatedForMerge->Lookup(sourceValueNumberPair, nullptr);
905+
if (newValue)
906+
{
907+
Assert(sameValueNumber == (newValue->GetValueNumber() == toDataValue->GetValueNumber()));
908+
909+
// This is an exception where Value::SetValueInfo is called directly instead of GlobOpt::ChangeValueInfo, because we're
910+
// actually generating new value info through merges.
911+
newValue->SetValueInfo(newValueInfo);
912+
}
913+
else
914+
{
915+
newValue = this->globOpt->NewValue(sameValueNumber ? sourceValueNumberPair.First() : this->globOpt->NewValueNumber(), newValueInfo);
916+
this->globOpt->valuesCreatedForMerge->Add(sourceValueNumberPair, newValue);
917+
}
898918
}
899919

900920
// Set symStore if same on both paths.

‎lib/Backend/GlobOptBlockData.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ class GlobOptBlockData
261261
template <typename CaptureList, typename CapturedItemsAreEqual>
262262
void MergeCapturedValues(SListBase<CaptureList>* toList, SListBase<CaptureList> * fromList, CapturedItemsAreEqual itemsAreEqual);
263263
void MergeValueMaps(BasicBlock *toBlock, BasicBlock *fromBlock, BVSparse<JitArenaAllocator> *const symsRequiringCompensation, BVSparse<JitArenaAllocator> *const symsCreatedForMerge);
264-
Value * MergeValues(Value *toDataValue, Value *fromDataValue, Sym *fromDataSym, bool isLoopBackEdge, BVSparse<JitArenaAllocator> *const symsRequiringCompensation, BVSparse<JitArenaAllocator> *const symsCreatedForMerge);
264+
Value * MergeValues(Value *toDataValue, Value *fromDataValue, Sym *fromDataSym, bool isLoopBackEdge, bool forceUniqueValue, BVSparse<JitArenaAllocator> *const symsRequiringCompensation, BVSparse<JitArenaAllocator> *const symsCreatedForMerge);
265265
ValueInfo * MergeValueInfo(Value *toDataVal, Value *fromDataVal, Sym *fromDataSym, bool isLoopBackEdge, bool sameValueNumber, BVSparse<JitArenaAllocator> *const symsRequiringCompensation, BVSparse<JitArenaAllocator> *const symsCreatedForMerge);
266266
JsTypeValueInfo * MergeJsTypeValueInfo(JsTypeValueInfo * toValueInfo, JsTypeValueInfo * fromValueInfo, bool isLoopBackEdge, bool sameValueNumber);
267267
ValueInfo * MergeArrayValueInfo(const ValueType mergedValueType, const ArrayValueInfo *const toDataValueInfo, const ArrayValueInfo *const fromDataValueInfo, Sym *const arraySym, BVSparse<JitArenaAllocator> *const symsRequiringCompensation, BVSparse<JitArenaAllocator> *const symsCreatedForMerge, bool isLoopBackEdge);

‎test/Optimizer/PrePassEntanglement.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
2+
function test() {
3+
var line = '"Value1InQuotes",Value2,Value 3 ,0.33,,,Last Value';
4+
var inQuotes = false;
5+
var quoted = false;
6+
for (var i = 0; i < line.length; i++) {
7+
if (inQuotes) {
8+
if (line[i] === '"') {
9+
inQuotes = false;
10+
}
11+
} else {
12+
if (line[i] === '"') {
13+
inQuotes = true;
14+
quoted = true;
15+
} else if (line[i] === ',') {
16+
if (line[i - 1] === '"' && !quoted) {
17+
WScript.Echo('Read from wrong var');
18+
return false;
19+
}
20+
}
21+
}
22+
}
23+
return true;
24+
}
25+
26+
if (test() && test() && test()) {
27+
WScript.Echo('Passed');
28+
}

‎test/Optimizer/rlexe.xml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1211,6 +1211,11 @@
12111211
<files>PrePassValues.js</files>
12121212
</default>
12131213
</test>
1214+
<test>
1215+
<default>
1216+
<files>PrePassEntanglement.js</files>
1217+
</default>
1218+
</test>
12141219
<test>
12151220
<default>
12161221
<files>missing_len.js</files>

‎test/PRE/pre1.baseline

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ testInlined
1919
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
2020
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
2121
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: FORWARD
22-
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: count
2322
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
2423
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: FORWARD
2524
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: count
@@ -30,7 +29,6 @@ undefined
3029
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
3130
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
3231
TestTrace fieldcopyprop [in landing pad]: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: FORWARD
33-
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: count
3432
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdRootFld field: Direction
3533
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: FORWARD
3634
TestTrace fieldcopyprop: function inlinee ( (#1.3), #4) inlined caller function testInlined ( (#1.4), #5) opcode: LdFld field: count

0 commit comments

Comments
 (0)