- Revision
- 248710
- Author
- ysuz...@apple.com
- Date
- 2019-08-14 23:39:28 -0700 (Wed, 14 Aug 2019)
Log Message
[JSC] Air does not appropriately propagate ConstFloatValue to stackmap
https://bugs.webkit.org/show_bug.cgi?id=200759
Reviewed by Saam Barati.
In B3MoveConstant phase, we convert ConstFloatValue and ConstDoubleValue to memory access to the table
to avoid large immediates *except for* stackmap argument case. This is because materializing constant doubles
and floats as memory-access before passing it to stackmap is wasteful: the stackmap may not use it actually, or
stackmap can do better job if it knows the parameter is constant.
Based on the above operation, B3LowerToAir phase strongly assumes that all ConstFloatValue and ConstDoubleValue
are removed except for the case used for parameter of stackmap. With r192377, B3LowerToAir catch this case, and
propagate constant double value as ValueRep in stackmap. While B3LowerToAir does this correctly for ConstDoubleValue,
we missed adding this support for ConstFloatValue.
This patch adds r192377's support for ConstFloatValue to propagate ConstFloatValue correctly to the stackmap.
This issue starts appearing since Wasm BBQ-B3 OSR starts putting ConstFloatValue to OSR-tier-up patchpoint.
* b3/B3LowerToAir.cpp:
* b3/B3ValueKey.h:
(JSC::B3::ValueKey::ValueKey):
(JSC::B3::ValueKey::floatValue const):
* b3/B3ValueRep.h:
(JSC::B3::ValueRep::constantFloat):
(JSC::B3::ValueRep::floatValue const):
* b3/testb3.h:
* b3/testb3_1.cpp:
(run):
* b3/testb3_5.cpp:
(testPatchpointManyWarmAnyImms):
(testPatchpointManyColdAnyImms):
(testPatchpointManyImms): Deleted.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (248709 => 248710)
--- trunk/Source/_javascript_Core/ChangeLog 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-08-15 06:39:28 UTC (rev 248710)
@@ -1,3 +1,38 @@
+2019-08-14 Yusuke Suzuki <ysuz...@apple.com>
+
+ [JSC] Air does not appropriately propagate ConstFloatValue to stackmap
+ https://bugs.webkit.org/show_bug.cgi?id=200759
+
+ Reviewed by Saam Barati.
+
+ In B3MoveConstant phase, we convert ConstFloatValue and ConstDoubleValue to memory access to the table
+ to avoid large immediates *except for* stackmap argument case. This is because materializing constant doubles
+ and floats as memory-access before passing it to stackmap is wasteful: the stackmap may not use it actually, or
+ stackmap can do better job if it knows the parameter is constant.
+
+ Based on the above operation, B3LowerToAir phase strongly assumes that all ConstFloatValue and ConstDoubleValue
+ are removed except for the case used for parameter of stackmap. With r192377, B3LowerToAir catch this case, and
+ propagate constant double value as ValueRep in stackmap. While B3LowerToAir does this correctly for ConstDoubleValue,
+ we missed adding this support for ConstFloatValue.
+
+ This patch adds r192377's support for ConstFloatValue to propagate ConstFloatValue correctly to the stackmap.
+ This issue starts appearing since Wasm BBQ-B3 OSR starts putting ConstFloatValue to OSR-tier-up patchpoint.
+
+ * b3/B3LowerToAir.cpp:
+ * b3/B3ValueKey.h:
+ (JSC::B3::ValueKey::ValueKey):
+ (JSC::B3::ValueKey::floatValue const):
+ * b3/B3ValueRep.h:
+ (JSC::B3::ValueRep::constantFloat):
+ (JSC::B3::ValueRep::floatValue const):
+ * b3/testb3.h:
+ * b3/testb3_1.cpp:
+ (run):
+ * b3/testb3_5.cpp:
+ (testPatchpointManyWarmAnyImms):
+ (testPatchpointManyColdAnyImms):
+ (testPatchpointManyImms): Deleted.
+
2019-08-14 Keith Rollin <krol...@apple.com>
Remove support for macOS < 10.13
Modified: trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2019-08-15 06:39:28 UTC (rev 248710)
@@ -1338,6 +1338,9 @@
else if (value.value()->hasDouble() && canBeInternal(value.value())) {
commitInternal(value.value());
arg = Arg::bigImm(bitwise_cast<int64_t>(value.value()->asDouble()));
+ } else if (value.value()->hasFloat() && canBeInternal(value.value())) {
+ commitInternal(value.value());
+ arg = Arg::bigImm(static_cast<uint64_t>(bitwise_cast<uint32_t>(value.value()->asFloat())));
} else
arg = tmp(value.value());
break;
Modified: trunk/Source/_javascript_Core/b3/B3ValueKey.h (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/B3ValueKey.h 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/B3ValueKey.h 2019-08-15 06:39:28 UTC (rev 248710)
@@ -80,6 +80,7 @@
: m_kind(kind)
, m_type(type)
{
+ // This means that upper 32bit of u.value is 0.
u.floatValue = value;
}
@@ -92,7 +93,7 @@
Value* child(Procedure&, unsigned index) const;
int64_t value() const { return u.value; }
double doubleValue() const { return u.doubleValue; }
- double floatValue() const { return u.floatValue; }
+ float floatValue() const { return u.floatValue; }
bool operator==(const ValueKey& other) const
{
Modified: trunk/Source/_javascript_Core/b3/B3ValueRep.h (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/B3ValueRep.h 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/B3ValueRep.h 2019-08-15 06:39:28 UTC (rev 248710)
@@ -159,6 +159,11 @@
return ValueRep::constant(bitwise_cast<int64_t>(value));
}
+ static ValueRep constantFloat(float value)
+ {
+ return ValueRep::constant(static_cast<uint64_t>(bitwise_cast<uint32_t>(value)));
+ }
+
Kind kind() const { return m_kind; }
bool operator==(const ValueRep& other) const
@@ -232,6 +237,11 @@
return bitwise_cast<double>(value());
}
+ float floatValue() const
+ {
+ return bitwise_cast<float>(static_cast<uint32_t>(static_cast<uint64_t>(value())));
+ }
+
ValueRep withOffset(intptr_t offset) const
{
switch (kind()) {
Modified: trunk/Source/_javascript_Core/b3/testb3.h (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/testb3.h 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/testb3.h 2019-08-15 06:39:28 UTC (rev 248710)
@@ -701,7 +701,8 @@
void testPatchpointFPScratch();
void testPatchpointLotsOfLateAnys();
void testPatchpointAnyImm(ValueRep rep);
-void testPatchpointManyImms();
+void testPatchpointManyWarmAnyImms();
+void testPatchpointManyColdAnyImms();
void testPatchpointWithRegisterResult();
void testPatchpointWithStackArgumentResult();
void testPatchpointWithAnyResult();
Modified: trunk/Source/_javascript_Core/b3/testb3_1.cpp (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/testb3_1.cpp 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/testb3_1.cpp 2019-08-15 06:39:28 UTC (rev 248710)
@@ -475,7 +475,8 @@
RUN(testPatchpointAnyImm(ValueRep::WarmAny));
RUN(testPatchpointAnyImm(ValueRep::ColdAny));
RUN(testPatchpointAnyImm(ValueRep::LateColdAny));
- RUN(testPatchpointManyImms());
+ RUN(testPatchpointManyWarmAnyImms());
+ RUN(testPatchpointManyColdAnyImms());
RUN(testPatchpointWithRegisterResult());
RUN(testPatchpointWithStackArgumentResult());
RUN(testPatchpointWithAnyResult());
Modified: trunk/Source/_javascript_Core/b3/testb3_5.cpp (248709 => 248710)
--- trunk/Source/_javascript_Core/b3/testb3_5.cpp 2019-08-15 06:20:11 UTC (rev 248709)
+++ trunk/Source/_javascript_Core/b3/testb3_5.cpp 2019-08-15 06:39:28 UTC (rev 248710)
@@ -28,7 +28,7 @@
#if ENABLE(B3_JIT)
-void testPatchpointManyImms()
+void testPatchpointManyWarmAnyImms()
{
Procedure proc;
BasicBlock* root = proc.addBlock();
@@ -36,18 +36,22 @@
Value* arg2 = root->appendNew<Const64Value>(proc, Origin(), 43);
Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
+ Value* arg5 = root->appendNew<ConstFloatValue>(proc, Origin(), -42.5f);
PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
patchpoint->append(ConstrainedValue(arg1, ValueRep::WarmAny));
patchpoint->append(ConstrainedValue(arg2, ValueRep::WarmAny));
patchpoint->append(ConstrainedValue(arg3, ValueRep::WarmAny));
patchpoint->append(ConstrainedValue(arg4, ValueRep::WarmAny));
+ patchpoint->append(ConstrainedValue(arg5, ValueRep::WarmAny));
patchpoint->setGenerator(
[&] (CCallHelpers&, const StackmapGenerationParams& params) {
- CHECK(params.size() == 4);
+ CHECK(params.size() == 5);
CHECK(params[0] == ValueRep::constant(42));
CHECK(params[1] == ValueRep::constant(43));
CHECK(params[2] == ValueRep::constant(43000000000000ll));
- CHECK(params[3] == ValueRep::constant(bitwise_cast<int64_t>(42.5)));
+ CHECK(params[3] == ValueRep::constantDouble(42.5));
+ CHECK(params[4] == ValueRep::constantFloat(-42.5f));
+ CHECK(params[4].floatValue() == -42.5f);
});
root->appendNewControlValue(
proc, Return, Origin(),
@@ -56,6 +60,38 @@
CHECK(!compileAndRun<int>(proc));
}
+void testPatchpointManyColdAnyImms()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ Value* arg1 = root->appendNew<Const32Value>(proc, Origin(), 42);
+ Value* arg2 = root->appendNew<Const64Value>(proc, Origin(), 43);
+ Value* arg3 = root->appendNew<Const64Value>(proc, Origin(), 43000000000000ll);
+ Value* arg4 = root->appendNew<ConstDoubleValue>(proc, Origin(), 42.5);
+ Value* arg5 = root->appendNew<ConstFloatValue>(proc, Origin(), -42.5f);
+ PatchpointValue* patchpoint = root->appendNew<PatchpointValue>(proc, Void, Origin());
+ patchpoint->append(ConstrainedValue(arg1, ValueRep::ColdAny));
+ patchpoint->append(ConstrainedValue(arg2, ValueRep::ColdAny));
+ patchpoint->append(ConstrainedValue(arg3, ValueRep::ColdAny));
+ patchpoint->append(ConstrainedValue(arg4, ValueRep::ColdAny));
+ patchpoint->append(ConstrainedValue(arg5, ValueRep::ColdAny));
+ patchpoint->setGenerator(
+ [&] (CCallHelpers&, const StackmapGenerationParams& params) {
+ CHECK(params.size() == 5);
+ CHECK(params[0] == ValueRep::constant(42));
+ CHECK(params[1] == ValueRep::constant(43));
+ CHECK(params[2] == ValueRep::constant(43000000000000ll));
+ CHECK(params[3] == ValueRep::constantDouble(42.5));
+ CHECK(params[4] == ValueRep::constantFloat(-42.5f));
+ CHECK(params[4].floatValue() == -42.5f);
+ });
+ root->appendNewControlValue(
+ proc, Return, Origin(),
+ root->appendNew<Const32Value>(proc, Origin(), 0));
+
+ CHECK(!compileAndRun<int>(proc));
+}
+
void testPatchpointWithRegisterResult()
{
Procedure proc;
