Modified: trunk/Source/_javascript_Core/ChangeLog (207162 => 207163)
--- trunk/Source/_javascript_Core/ChangeLog 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-10-11 20:37:51 UTC (rev 207163)
@@ -1,3 +1,40 @@
+2016-10-11 Filip Pizlo <[email protected]>
+
+ B3->Air lowering needs the same defenses in effectiveAddr() that it has in tryAppendLea()
+ https://bugs.webkit.org/show_bug.cgi?id=163264
+
+ Reviewed by Mark Lam.
+
+ When writing the lea patch (r207039), I was very careful about how I convert a Shl into a
+ BaseIndex scale. But I forgot to check if the older code for creating BaseIndexes for
+ effectiveAddr() got this right. It turns out that the older code missed the <<32 corner
+ case.
+
+ It's sad that the two paths can't share all of their code, but it's somewhat inevitable due
+ to how matching an address and matching a lea have to do very different things. Matching a
+ lea means creating an instruction that is distinct from other instructions to do multiple
+ math operations at once. Matching an address means making some instruction do extra work
+ for free. Also, address matching can take advantage of the fact that the offset is already
+ associated with the memory operation by strength reduction - lea matching can't do this; it
+ has to figure out Add(@value, $const) on its own. This change makes the situation slightly
+ more sane by adding a scaleForShl() helper that handles this weird case. It's based on the
+ new Shl handling from r207039, and exposes it as an API for effectiveAddr() to use.
+
+ The testLoadBaseIndexShift32() used to crash. I don't think that this case affects JS
+ content, since <<32 is such a bizarre outlier. I don't think we even have a path along
+ which the FTL would emit a 64-bit <<32. It probably won't even affect WebAssembly since
+ that uses 32-bit pointers, so we won't see 64-bit <<32 in effectiveAddr() there.
+
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::scaleForShl):
+ (JSC::B3::Air::LowerToAir::effectiveAddr):
+ (JSC::B3::Air::LowerToAir::tryAppendLea):
+ (JSC::B3::Air::LowerToAir::crossesInterference): Deleted.
+ * b3/testb3.cpp:
+ (JSC::B3::testLoadBaseIndexShift2):
+ (JSC::B3::testLoadBaseIndexShift32):
+ (JSC::B3::run):
+
2016-10-11 Saam Barati <[email protected]>
ValueAdd should be constant folded if the operands are constant String,Primitive or Primitive,String
Modified: trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp (207162 => 207163)
--- trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2016-10-11 20:37:51 UTC (rev 207163)
@@ -425,6 +425,28 @@
ASSERT_NOT_REACHED();
return true;
}
+
+ Optional<unsigned> scaleForShl(Value* shl, int32_t offset, Optional<Arg::Width> width = Nullopt)
+ {
+ if (shl->opcode() != Shl)
+ return Nullopt;
+ if (!shl->child(1)->hasInt32())
+ return Nullopt;
+ unsigned logScale = shl->child(1)->asInt32();
+ if (shl->type() == Int32)
+ logScale &= 31;
+ else
+ logScale &= 63;
+ // Use 64-bit math to perform the shift so that <<32 does the right thing, but then switch
+ // to signed since that's what all of our APIs want.
+ int64_t bigScale = static_cast<uint64_t>(1) << static_cast<uint64_t>(logScale);
+ if (!isRepresentableAs<int32_t>(bigScale))
+ return Nullopt;
+ unsigned scale = static_cast<int32_t>(bigScale);
+ if (!Arg::isValidIndexForm(scale, offset, width))
+ return Nullopt;
+ return scale;
+ }
// This turns the given operand into an address.
Arg effectiveAddr(Value* address, int32_t offset, Arg::Width width)
@@ -447,18 +469,12 @@
Value* right = address->child(1);
auto tryIndex = [&] (Value* index, Value* base) -> Arg {
- if (index->opcode() != Shl)
+ Optional<unsigned> scale = scaleForShl(index, offset, width);
+ if (!scale)
return Arg();
if (m_locked.contains(index->child(0)) || m_locked.contains(base))
return Arg();
- if (!index->child(1)->hasInt32())
- return Arg();
-
- unsigned scale = 1 << (index->child(1)->asInt32() & 31);
- if (!Arg::isValidIndexForm(scale, offset, width))
- return Arg();
-
- return Arg::index(tmp(base), tmp(index->child(0)), scale, offset);
+ return Arg::index(tmp(base), tmp(index->child(0)), *scale, offset);
};
if (Arg result = tryIndex(left, right))
@@ -1898,29 +1914,16 @@
}
auto tryShl = [&] (Value* shl, Value* other) -> bool {
- if (shl->opcode() != Shl)
+ Optional<unsigned> scale = scaleForShl(shl, offset);
+ if (!scale)
return false;
if (!canBeInternal(shl))
return false;
- if (!shl->child(1)->hasInt32())
- return false;
- unsigned logScale = shl->child(1)->asInt32();
- if (m_value->type() == Int32)
- logScale &= 31;
- else
- logScale &= 63;
- // Use 64-bit math to perform the shift so that <<32 does the right thing.
- int64_t bigScale = static_cast<uint64_t>(1) << static_cast<uint64_t>(logScale);
- if (!isRepresentableAs<int32_t>(bigScale))
- return false;
- unsigned scale = static_cast<int32_t>(bigScale);
- if (!Arg::isValidIndexForm(scale, offset))
- return false;
ASSERT(!m_locked.contains(shl->child(0)));
ASSERT(!m_locked.contains(other));
- append(leaOpcode, Arg::index(tmp(other), tmp(shl->child(0)), scale, offset), tmp(m_value));
+ append(leaOpcode, Arg::index(tmp(other), tmp(shl->child(0)), *scale, offset), tmp(m_value));
commitInternal(innerAdd);
commitInternal(shl);
return true;
Modified: trunk/Source/_javascript_Core/b3/testb3.cpp (207162 => 207163)
--- trunk/Source/_javascript_Core/b3/testb3.cpp 2016-10-11 20:27:39 UTC (rev 207162)
+++ trunk/Source/_javascript_Core/b3/testb3.cpp 2016-10-11 20:37:51 UTC (rev 207163)
@@ -13659,6 +13659,52 @@
(root->last()->child(0)->child(0)->child(0) == arg2 && root->last()->child(0)->child(0)->child(1) == arg1));
}
+void testLoadBaseIndexShift2()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ root->appendNew<Value>(
+ proc, Return, Origin(),
+ root->appendNew<MemoryValue>(
+ proc, Load, Int32, Origin(),
+ root->appendNew<Value>(
+ proc, Add, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0),
+ root->appendNew<Value>(
+ proc, Shl, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1),
+ root->appendNew<Const32Value>(proc, Origin(), 2)))));
+ auto code = compile(proc);
+ if (isX86())
+ checkUsesInstruction(*code, "(%rdi,%rsi,4)");
+ int32_t value = 12341234;
+ char* ptr = bitwise_cast<char*>(&value);
+ for (unsigned i = 0; i < 10; ++i)
+ CHECK_EQ(invoke<int32_t>(*code, ptr - (static_cast<intptr_t>(1) << static_cast<intptr_t>(2)) * i, i), 12341234);
+}
+
+void testLoadBaseIndexShift32()
+{
+ Procedure proc;
+ BasicBlock* root = proc.addBlock();
+ root->appendNew<Value>(
+ proc, Return, Origin(),
+ root->appendNew<MemoryValue>(
+ proc, Load, Int32, Origin(),
+ root->appendNew<Value>(
+ proc, Add, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0),
+ root->appendNew<Value>(
+ proc, Shl, Origin(),
+ root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1),
+ root->appendNew<Const32Value>(proc, Origin(), 32)))));
+ auto code = compile(proc);
+ int32_t value = 12341234;
+ char* ptr = bitwise_cast<char*>(&value);
+ for (unsigned i = 0; i < 10; ++i)
+ CHECK_EQ(invoke<int32_t>(*code, ptr - (static_cast<intptr_t>(1) << static_cast<intptr_t>(32)) * i, i), 12341234);
+}
+
// Make sure the compiler does not try to optimize anything out.
NEVER_INLINE double zero()
{
@@ -15095,6 +15141,8 @@
RUN(testAddShl32());
RUN(testAddShl64());
RUN(testAddShl65());
+ RUN(testLoadBaseIndexShift2());
+ RUN(testLoadBaseIndexShift32());
if (isX86()) {
RUN(testBranchBitAndImmFusion(Identity, Int64, 1, Air::BranchTest32, Air::Arg::Tmp));
