- Revision
- 278810
- Author
- rmoris...@apple.com
- Date
- 2021-06-12 10:17:27 -0700 (Sat, 12 Jun 2021)
Log Message
We should drop B3 values while running Air
https://bugs.webkit.org/show_bug.cgi?id=226187
Reviewed by Saam Barati.
NB: this patch first landed as r278371, then was reverted in r278587 as it broke --dumpDisassembly().
I fixed the bug (a missing line setDisassembler()) and am now re-landing it. Below is a copy of the original Changelog.
We must keep the following values:
- WasmBoundsCheck, to know whether it is Pinned or Maximum, and if it is pinned find its argument.
- CCall/Patch/Check/CheckAdd/CheckSub/CheckMul and all of their children, because all of these are lowered to Air::Patchpoint, which needs to know the type of its arguments, and does so by looking at the children of its origin.
I intend to fix these in later patches if possible.
Finally we must preserve all B3 values in the following cases:
- if we dump the disassembly or the Air graph: because otherwise we cannot print the origins
- if we are using the sampling profiler, because it relies on PCToCodeOriginMap which we cannot accurately fill without these origins.
We must also keep m_tuples alive, as it is used by Patchpoints in Air to understand the types of their arguments.
We also don't touch StackSlots (in this patch), because one of them is captured by FTL::State.
Also now PCToOriginMap has a Vector with no inline capacity, since it is either quite large (if needed) or empty (otherwise).
The performance impact of this is a progression on various RAMification subtests on Mac, but is more mitigated on iPhone7, with various regressions.
I suspect these to be noise, and will monitor the performance bots post-landing to make sure of it.
* b3/B3LowerToAir.cpp:
(JSC::B3::lowerToAir):
* b3/B3Procedure.cpp:
(JSC::B3::Procedure::freeUnneededB3ValuesAfterLowering):
* b3/B3Procedure.h:
(JSC::B3::Procedure::releasePCToOriginMap):
(JSC::B3::Procedure::setNeedsPCToOriginMap):
(JSC::B3::Procedure::needsPCToOriginMap):
* b3/B3SparseCollection.h:
(JSC::B3::SparseCollection::clearAll):
(JSC::B3::SparseCollection::filterAndTransfer):
* b3/air/AirCode.cpp:
(JSC::B3::Air::Code::Code):
* b3/air/AirCode.h:
(JSC::B3::Air::Code::shouldPreserveB3Origins const):
* b3/air/AirGenerate.cpp:
(JSC::B3::Air::generateWithAlreadyAllocatedRegisters):
* ftl/FTLCompile.cpp:
(JSC::FTL::compile):
* ftl/FTLState.cpp:
(JSC::FTL::State::State):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (278809 => 278810)
--- trunk/Source/_javascript_Core/ChangeLog 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,3 +1,52 @@
+2021-06-12 Robin Morisset <rmoris...@apple.com>
+
+ We should drop B3 values while running Air
+ https://bugs.webkit.org/show_bug.cgi?id=226187
+
+ Reviewed by Saam Barati.
+
+ NB: this patch first landed as r278371, then was reverted in r278587 as it broke --dumpDisassembly().
+ I fixed the bug (a missing line setDisassembler()) and am now re-landing it. Below is a copy of the original Changelog.
+
+ We must keep the following values:
+ - WasmBoundsCheck, to know whether it is Pinned or Maximum, and if it is pinned find its argument.
+ - CCall/Patch/Check/CheckAdd/CheckSub/CheckMul and all of their children, because all of these are lowered to Air::Patchpoint, which needs to know the type of its arguments, and does so by looking at the children of its origin.
+ I intend to fix these in later patches if possible.
+
+ Finally we must preserve all B3 values in the following cases:
+ - if we dump the disassembly or the Air graph: because otherwise we cannot print the origins
+ - if we are using the sampling profiler, because it relies on PCToCodeOriginMap which we cannot accurately fill without these origins.
+
+ We must also keep m_tuples alive, as it is used by Patchpoints in Air to understand the types of their arguments.
+ We also don't touch StackSlots (in this patch), because one of them is captured by FTL::State.
+
+ Also now PCToOriginMap has a Vector with no inline capacity, since it is either quite large (if needed) or empty (otherwise).
+
+ The performance impact of this is a progression on various RAMification subtests on Mac, but is more mitigated on iPhone7, with various regressions.
+ I suspect these to be noise, and will monitor the performance bots post-landing to make sure of it.
+
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::lowerToAir):
+ * b3/B3Procedure.cpp:
+ (JSC::B3::Procedure::freeUnneededB3ValuesAfterLowering):
+ * b3/B3Procedure.h:
+ (JSC::B3::Procedure::releasePCToOriginMap):
+ (JSC::B3::Procedure::setNeedsPCToOriginMap):
+ (JSC::B3::Procedure::needsPCToOriginMap):
+ * b3/B3SparseCollection.h:
+ (JSC::B3::SparseCollection::clearAll):
+ (JSC::B3::SparseCollection::filterAndTransfer):
+ * b3/air/AirCode.cpp:
+ (JSC::B3::Air::Code::Code):
+ * b3/air/AirCode.h:
+ (JSC::B3::Air::Code::shouldPreserveB3Origins const):
+ * b3/air/AirGenerate.cpp:
+ (JSC::B3::Air::generateWithAlreadyAllocatedRegisters):
+ * ftl/FTLCompile.cpp:
+ (JSC::FTL::compile):
+ * ftl/FTLState.cpp:
+ (JSC::FTL::State::State):
+
2021-06-11 Patrick Angle <pan...@apple.com>
Web Inspector: Add instrumentation to node destruction for InspectorDOMAgent
Modified: trunk/Source/_javascript_Core/b3/B3Generate.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/B3Generate.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/B3Generate.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -133,6 +133,7 @@
}
lowerToAir(procedure);
+ procedure.freeUnneededB3ValuesAfterLowering();
}
} } // namespace JSC::B3
Modified: trunk/Source/_javascript_Core/b3/B3PCToOriginMap.h (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/B3PCToOriginMap.h 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/B3PCToOriginMap.h 2021-06-12 17:17:27 UTC (rev 278810)
@@ -61,7 +61,7 @@
const Vector<OriginRange>& ranges() const { return m_ranges; }
private:
- Vector<OriginRange> m_ranges;
+ Vector<OriginRange, 0> m_ranges;
};
} } // namespace JSC::B3
Modified: trunk/Source/_javascript_Core/b3/B3Procedure.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/B3Procedure.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/B3Procedure.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2015-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -434,6 +434,51 @@
m_code->setNumEntrypoints(numEntrypoints);
}
+void Procedure::freeUnneededB3ValuesAfterLowering()
+{
+ // We cannot clear m_stackSlots() or m_tuples here, as they are unfortunately modified and read respectively by Air.
+ m_variables.clearAll();
+ m_blocks.clear();
+ m_cfg = nullptr;
+ m_dominators = nullptr;
+ m_naturalLoops = nullptr;
+ m_backwardsCFG = nullptr;
+ m_backwardsDominators = nullptr;
+ m_fastConstants.clear();
+
+ if (m_code->shouldPreserveB3Origins())
+ return;
+
+ BitVector valuesToPreserve;
+ valuesToPreserve.ensureSize(m_values.size());
+ for (Value* value : m_values) {
+ switch (value->opcode()) {
+ // Ideally we would also be able to get rid of all of those.
+ // But Air currently relies on these origins being preserved, see https://bugs.webkit.org/show_bug.cgi?id=194040
+ case WasmBoundsCheck:
+ valuesToPreserve.quickSet(value->index());
+ break;
+ case CCall:
+ case Patchpoint:
+ case CheckAdd:
+ case CheckSub:
+ case CheckMul:
+ case Check:
+ valuesToPreserve.quickSet(value->index());
+ for (Value* child : value->children())
+ valuesToPreserve.quickSet(child->index());
+ break;
+ default:
+ break;
+ }
+ }
+ for (Value* value : m_values) {
+ if (!valuesToPreserve.quickGet(value->index()))
+ m_values.remove(value);
+ }
+ m_values.packIndices();
+}
+
} } // namespace JSC::B3
#endif // ENABLE(B3_JIT)
Modified: trunk/Source/_javascript_Core/b3/B3Procedure.h (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/B3Procedure.h 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/B3Procedure.h 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -258,7 +258,11 @@
JS_EXPORT_PRIVATE RegisterAtOffsetList calleeSaveRegisterAtOffsetList() const;
PCToOriginMap& pcToOriginMap() { return m_pcToOriginMap; }
- PCToOriginMap releasePCToOriginMap() { return WTFMove(m_pcToOriginMap); }
+ PCToOriginMap releasePCToOriginMap()
+ {
+ RELEASE_ASSERT(needsPCToOriginMap());
+ return WTFMove(m_pcToOriginMap);
+ }
JS_EXPORT_PRIVATE void setWasmBoundsCheckGenerator(RefPtr<WasmBoundsCheckGenerator>);
@@ -271,6 +275,11 @@
JS_EXPORT_PRIVATE RegisterSet mutableGPRs();
JS_EXPORT_PRIVATE RegisterSet mutableFPRs();
+ void setNeedsPCToOriginMap() { m_needsPCToOriginMap = true; }
+ bool needsPCToOriginMap() { return m_needsPCToOriginMap; }
+
+ JS_EXPORT_PRIVATE void freeUnneededB3ValuesAfterLowering();
+
private:
friend class BlockInsertionSet;
@@ -287,7 +296,6 @@
std::unique_ptr<BackwardsCFG> m_backwardsCFG;
std::unique_ptr<BackwardsDominators> m_backwardsDominators;
HashSet<ValueKey> m_fastConstants;
- unsigned m_numEntrypoints { 1 };
const char* m_lastPhaseName;
std::unique_ptr<OpaqueByproducts> m_byproducts;
std::unique_ptr<Air::Code> m_code;
@@ -294,9 +302,11 @@
RefPtr<SharedTask<void(PrintStream&, Origin)>> m_originPrinter;
const void* m_frontendData;
PCToOriginMap m_pcToOriginMap;
+ unsigned m_numEntrypoints { 1 };
unsigned m_optLevel { defaultOptLevel() };
bool m_needsUsedRegisters { true };
bool m_hasQuirks { false };
+ bool m_needsPCToOriginMap { false };
};
} } // namespace JSC::B3
Modified: trunk/Source/_javascript_Core/b3/B3SparseCollection.h (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/B3SparseCollection.h 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/B3SparseCollection.h 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -111,6 +111,12 @@
m_vector.shrink(endIndex);
}
+ void clearAll()
+ {
+ m_indexFreeList.clear();
+ m_vector.clear();
+ }
+
unsigned size() const { return m_vector.size(); }
bool isEmpty() const { return m_vector.isEmpty(); }
@@ -154,6 +160,8 @@
}
private:
+ friend class SparseCollection;
+
unsigned findNext(unsigned index)
{
while (index < m_collection->size() && !m_collection->at(index))
Modified: trunk/Source/_javascript_Core/b3/air/AirCode.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/air/AirCode.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/air/AirCode.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -56,6 +56,7 @@
Code::Code(Procedure& proc)
: m_proc(proc)
, m_cfg(new CFG(*this))
+ , m_preserveB3Origins(proc.needsPCToOriginMap() || Options::dumpAirGraphAtEachPhase() || Options::dumpFTLDisassembly())
, m_lastPhaseName("initial")
, m_defaultPrologueGenerator(createSharedTask<PrologueGeneratorFunction>(&defaultPrologueGenerator))
{
Modified: trunk/Source/_javascript_Core/b3/air/AirCode.h (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/air/AirCode.h 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/air/AirCode.h 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2015-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -341,7 +341,14 @@
// it's mainly for validating the results from JSAir.
unsigned jsHash() const;
- void setDisassembler(std::unique_ptr<Disassembler>&& disassembler) { m_disassembler = WTFMove(disassembler); }
+ bool shouldPreserveB3Origins() const { return m_preserveB3Origins; }
+ void forcePreservationOfB3Origins() { m_preserveB3Origins = true; }
+
+ void setDisassembler(std::unique_ptr<Disassembler>&& disassembler)
+ {
+ m_disassembler = WTFMove(disassembler);
+ forcePreservationOfB3Origins();
+ }
Disassembler* disassembler() { return m_disassembler.get(); }
RegisterSet mutableGPRs();
@@ -390,7 +397,9 @@
unsigned m_numFPTmps { 0 };
unsigned m_frameSize { 0 };
unsigned m_callArgAreaSize { 0 };
+ unsigned m_optLevel { defaultOptLevel() };
bool m_stackIsAllocated { false };
+ bool m_preserveB3Origins { true };
RegisterAtOffsetList m_uncorrectedCalleeSaveRegisterAtOffsetList;
RegisterSet m_calleeSaveRegisters;
StackSlot* m_calleeSaveStackSlot { nullptr };
@@ -400,7 +409,6 @@
RefPtr<WasmBoundsCheckGenerator> m_wasmBoundsCheckGenerator;
const char* m_lastPhaseName;
std::unique_ptr<Disassembler> m_disassembler;
- unsigned m_optLevel { defaultOptLevel() };
Ref<PrologueGenerator> m_defaultPrologueGenerator;
};
Modified: trunk/Source/_javascript_Core/b3/air/AirGenerate.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/air/AirGenerate.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/air/AirGenerate.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2015-2020 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2021 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -214,11 +214,12 @@
PCToOriginMap& pcToOriginMap = code.proc().pcToOriginMap();
auto addItem = [&] (Inst& inst) {
- if (!inst.origin) {
+ if (!code.shouldPreserveB3Origins())
+ return;
+ if (inst.origin)
+ pcToOriginMap.appendItem(jit.labelIgnoringWatchpoints(), inst.origin->origin());
+ else
pcToOriginMap.appendItem(jit.labelIgnoringWatchpoints(), Origin());
- return;
- }
- pcToOriginMap.appendItem(jit.labelIgnoringWatchpoints(), inst.origin->origin());
};
Disassembler* disassembler = code.disassembler();
Modified: trunk/Source/_javascript_Core/b3/testb3_6.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/b3/testb3_6.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/b3/testb3_6.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -2767,6 +2767,8 @@
root->appendNew<Const32Value>(proc, Origin(), 44),
ptr);
root->appendNew<Value>(proc, Return, Origin());
+ // We'll look at the values after compiling
+ proc.code().forcePreservationOfB3Origins();
compileAndRun<int>(proc);
unsigned storeCount = 0;
for (Value* value : proc.values()) {
Modified: trunk/Source/_javascript_Core/ftl/FTLCompile.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/ftl/FTLCompile.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/ftl/FTLCompile.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -150,10 +150,11 @@
state.allocationFailed = true;
return;
}
-
- B3::PCToOriginMap originMap = state.proc->releasePCToOriginMap();
- if (vm.shouldBuilderPCToCodeOriginMapping())
+
+ if (vm.shouldBuilderPCToCodeOriginMapping()) {
+ B3::PCToOriginMap originMap = state.proc->releasePCToOriginMap();
codeBlock->setPCToCodeOriginMap(makeUnique<PCToCodeOriginMap>(PCToCodeOriginMapBuilder(vm, WTFMove(originMap)), *state.finalizer->b3CodeLinkBuffer));
+ }
CodeLocationLabel<JSEntryPtrTag> label = state.finalizer->b3CodeLinkBuffer->locationOf<JSEntryPtrTag>(state.proc->code().entrypointLabel(0));
state.generatedFunction = label;
Modified: trunk/Source/_javascript_Core/ftl/FTLState.cpp (278809 => 278810)
--- trunk/Source/_javascript_Core/ftl/FTLState.cpp 2021-06-12 07:26:33 UTC (rev 278809)
+++ trunk/Source/_javascript_Core/ftl/FTLState.cpp 2021-06-12 17:17:27 UTC (rev 278810)
@@ -63,6 +63,9 @@
proc = makeUnique<Procedure>();
+ if (graph.m_vm.shouldBuilderPCToCodeOriginMapping())
+ proc->setNeedsPCToOriginMap();
+
proc->setOriginPrinter(
[] (PrintStream& out, B3::Origin origin) {
out.print(bitwise_cast<Node*>(origin.data()));
