Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (194400 => 194401)
--- trunk/Source/_javascript_Core/ChangeLog 2015-12-23 23:45:17 UTC (rev 194400)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-12-24 00:14:13 UTC (rev 194401)
@@ -1,3 +1,39 @@
+2015-12-23 Filip Pizlo <[email protected]>
+
+ Need a story for platform-specific Args
+ https://bugs.webkit.org/show_bug.cgi?id=152529
+
+ Reviewed by Michael Saboff.
+
+ This teaches Arg that some Arg forms are not valid on some targets. The instruction selector now
+ uses this to avoid immediates and addresses that the target wouldn't like.
+
+ This shouldn't change code generation on X86, but is meant as a step towards ARM64 support.
+
+ * b3/B3LowerToAir.cpp:
+ (JSC::B3::Air::LowerToAir::crossesInterference):
+ (JSC::B3::Air::LowerToAir::effectiveAddr):
+ (JSC::B3::Air::LowerToAir::addr):
+ (JSC::B3::Air::LowerToAir::loadPromise):
+ (JSC::B3::Air::LowerToAir::imm):
+ (JSC::B3::Air::LowerToAir::lower):
+ * b3/air/AirAllocateStack.cpp:
+ (JSC::B3::Air::allocateStack):
+ * b3/air/AirArg.h:
+ (JSC::B3::Air::Arg::Arg):
+ (JSC::B3::Air::Arg::imm):
+ (JSC::B3::Air::Arg::imm64):
+ (JSC::B3::Air::Arg::callArg):
+ (JSC::B3::Air::Arg::isValidScale):
+ (JSC::B3::Air::Arg::tmpIndex):
+ (JSC::B3::Air::Arg::withOffset):
+ (JSC::B3::Air::Arg::isValidImmForm):
+ (JSC::B3::Air::Arg::isValidAddrForm):
+ (JSC::B3::Air::Arg::isValidIndexForm):
+ (JSC::B3::Air::Arg::isValidForm):
+ (JSC::B3::Air::Arg::forEachTmpFast):
+ * b3/air/opcode_generator.rb:
+
2015-12-23 Keith Miller <[email protected]>
[JSC] Bugfix for intrinsic getters with dictionary structures.
Modified: trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp (194400 => 194401)
--- trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2015-12-23 23:45:17 UTC (rev 194400)
+++ trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2015-12-24 00:14:13 UTC (rev 194401)
@@ -337,32 +337,42 @@
}
// This turns the given operand into an address.
- Arg effectiveAddr(Value* address)
+ Arg effectiveAddr(Value* address, int32_t offset, Arg::Width width)
{
+ // B3 allows any memory operation to have a 32-bit offset. That's not how some architectures
+ // work. We solve this by requiring a just-before-lowering phase that legalizes offsets.
+ // FIXME: Implement such a legalization phase.
+ // https://bugs.webkit.org/show_bug.cgi?id=152530
+ ASSERT(Arg::isValidAddrForm(offset));
+
+ auto fallback = [&] () -> Arg {
+ return Arg::addr(tmp(address), offset);
+ };
+
static const unsigned lotsOfUses = 10; // This is arbitrary and we should tune it eventually.
-
+
// Only match if the address value isn't used in some large number of places.
if (m_useCounts.numUses(address) > lotsOfUses)
- return Arg::addr(tmp(address));
+ return fallback();
switch (address->opcode()) {
case Add: {
Value* left = address->child(0);
Value* right = address->child(1);
- auto tryIndex = [&] (Value* index, Value* offset) -> Arg {
+ auto tryIndex = [&] (Value* index, Value* base) -> Arg {
if (index->opcode() != Shl)
return Arg();
- if (m_locked.contains(index->child(0)) || m_locked.contains(offset))
+ 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::isValidScale(scale))
+ if (!Arg::isValidIndexForm(scale, offset, width))
return Arg();
- return Arg::index(tmp(offset), tmp(index->child(0)), scale);
+ return Arg::index(tmp(base), tmp(index->child(0)), scale, offset);
};
if (Arg result = tryIndex(left, right))
@@ -370,10 +380,11 @@
if (Arg result = tryIndex(right, left))
return result;
- if (m_locked.contains(left) || m_locked.contains(right))
- return Arg::addr(tmp(address));
+ if (m_locked.contains(left) || m_locked.contains(right)
+ || !Arg::isValidIndexForm(1, offset, width))
+ return fallback();
- return Arg::index(tmp(left), tmp(right));
+ return Arg::index(tmp(left), tmp(right), 1, offset);
}
case Shl: {
@@ -382,20 +393,21 @@
// We'll never see child(1)->isInt32(0), since that would have been reduced. If the shift
// amount is greater than 1, then there isn't really anything smart that we could do here.
// We avoid using baseless indexes because their encoding isn't particularly efficient.
- if (m_locked.contains(left) || !address->child(1)->isInt32(1))
- return Arg::addr(tmp(address));
+ if (m_locked.contains(left) || !address->child(1)->isInt32(1)
+ || !Arg::isValidIndexForm(1, offset, width))
+ return fallback();
- return Arg::index(tmp(left), tmp(left));
+ return Arg::index(tmp(left), tmp(left), 1, offset);
}
case FramePointer:
- return Arg::addr(Tmp(GPRInfo::callFrameRegister));
+ return Arg::addr(Tmp(GPRInfo::callFrameRegister), offset);
case B3::StackSlot:
- return Arg::stack(m_stackToStack.get(address->as<StackSlotValue>()));
+ return Arg::stack(m_stackToStack.get(address->as<StackSlotValue>()), offset);
default:
- return Arg::addr(tmp(address));
+ return fallback();
}
}
@@ -407,14 +419,13 @@
if (!value)
return Arg();
- Arg result = effectiveAddr(value->lastChild());
- ASSERT(result);
-
- int32_t offset = memoryValue->as<MemoryValue>()->offset();
- Arg offsetResult = result.withOffset(offset);
- if (!offsetResult)
- return Arg::addr(tmp(value->lastChild()), offset);
- return offsetResult;
+ int32_t offset = value->offset();
+ Arg::Width width = Arg::widthForBytes(value->accessByteSize());
+
+ Arg result = effectiveAddr(value->lastChild(), offset, width);
+ ASSERT(result.isValidForm(width));
+
+ return result;
}
ArgPromise loadPromise(Value* loadValue, B3::Opcode loadOpcode)
@@ -435,8 +446,11 @@
Arg imm(Value* value)
{
- if (value->hasInt() && value->representableAs<int32_t>())
- return Arg::imm(value->asNumber<int32_t>());
+ if (value->hasInt()) {
+ int64_t intValue = value->asInt();
+ if (Arg::isValidImmForm(intValue))
+ return Arg::imm(intValue);
+ }
return Arg();
}
@@ -1798,15 +1812,12 @@
return;
}
- case Const32: {
- append(Move, imm(m_value), tmp(m_value));
- return;
- }
+ case Const32:
case Const64: {
if (imm(m_value))
append(Move, imm(m_value), tmp(m_value));
else
- append(Move, Arg::imm64(m_value->asInt64()), tmp(m_value));
+ append(Move, Arg::imm64(m_value->asInt()), tmp(m_value));
return;
}
Modified: trunk/Source/_javascript_Core/b3/air/AirAllocateStack.cpp (194400 => 194401)
--- trunk/Source/_javascript_Core/b3/air/AirAllocateStack.cpp 2015-12-23 23:45:17 UTC (rev 194400)
+++ trunk/Source/_javascript_Core/b3/air/AirAllocateStack.cpp 2015-12-24 00:14:13 UTC (rev 194401)
@@ -231,6 +231,11 @@
// transformation since we can search the StackSlots array to figure out which StackSlot any
// offset-from-FP refers to.
+ // FIXME: This may produce addresses that aren't valid if we end up with a ginormous stack frame.
+ // We would have to scavenge for temporaries if this happened. Fortunately, this case will be
+ // extremely rare so we can do crazy things when it arises.
+ // https://bugs.webkit.org/show_bug.cgi?id=152530
+
for (BasicBlock* block : code) {
for (Inst& inst : *block) {
for (Arg& arg : inst.args) {
Modified: trunk/Source/_javascript_Core/b3/air/AirArg.h (194400 => 194401)
--- trunk/Source/_javascript_Core/b3/air/AirArg.h 2015-12-23 23:45:17 UTC (rev 194400)
+++ trunk/Source/_javascript_Core/b3/air/AirArg.h 2015-12-24 00:14:13 UTC (rev 194401)
@@ -31,6 +31,7 @@
#include "AirTmp.h"
#include "B3Common.h"
#include "B3Type.h"
+#include <wtf/Optional.h>
namespace JSC { namespace B3 { namespace Air {
@@ -49,7 +50,10 @@
// eventually become registers.
Tmp,
- // This is an immediate that the instruction will materialize.
+ // This is an immediate that the instruction will materialize. Imm is the immediate that can be
+ // inlined into most instructions, while Imm64 indicates a constant materialization and is
+ // usually only usable with Move. Specials may also admit it, for example for stackmaps used for
+ // OSR exit and tail calls.
Imm,
Imm64,
@@ -327,7 +331,7 @@
{
}
- static Arg imm(int32_t value)
+ static Arg imm(int64_t value)
{
Arg result;
result.m_kind = Imm;
@@ -335,7 +339,7 @@
return result;
}
- static Arg imm64(intptr_t value)
+ static Arg imm64(int64_t value)
{
Arg result;
result.m_kind = Imm64;
@@ -370,14 +374,25 @@
return result;
}
- static bool isValidScale(unsigned scale)
+ // If you don't pass a Width, this optimistically assumes that you're using the right width.
+ static bool isValidScale(unsigned scale, Optional<Width> width = Nullopt)
{
switch (scale) {
case 1:
+ if (isX86() || isARM64())
+ return true;
+ return false;
case 2:
case 4:
case 8:
- return true;
+ if (isX86())
+ return true;
+ if (isARM64()) {
+ if (!width)
+ return true;
+ return scale == 1 || scale == bytes(*width);
+ }
+ return false;
default:
return false;
}
@@ -754,11 +769,17 @@
return tmp().tmpIndex();
}
- Arg withOffset(int32_t additionalOffset) const
+ // If 'this' is an address Arg, then it returns a new address Arg with the additional offset applied.
+ // Note that this does not consider whether doing so produces a valid Arg or not. Unless you really
+ // know what you're doing, you should call Arg::isValidForm() on the result. Some code won't do that,
+ // like if you're applying a very small offset to a Arg::stack() that you know has no offset to begin
+ // with. It's safe to assume that all targets allow small offsets (like, 0..7) for Addr, Stack, and
+ // CallArg.
+ Arg withOffset(int64_t additionalOffset) const
{
if (!hasOffset())
return Arg();
- if (sumOverflows<int32_t>(offset(), additionalOffset))
+ if (sumOverflows<int64_t>(offset(), additionalOffset))
return Arg();
switch (kind()) {
case Addr:
@@ -775,6 +796,64 @@
}
}
+ static bool isValidImmForm(int64_t value)
+ {
+ if (isX86())
+ return B3::isRepresentableAs<int32_t>(value);
+ // FIXME: ARM has some specific rules about what kinds of immediates are valid.
+ // https://bugs.webkit.org/show_bug.cgi?id=152530
+ return false;
+ }
+
+ static bool isValidAddrForm(int32_t offset)
+ {
+ if (isX86())
+ return true;
+ // FIXME: ARM has some specific rules about what kinds of offsets are valid.
+ // https://bugs.webkit.org/show_bug.cgi?id=152530
+ UNUSED_PARAM(offset);
+ return false;
+ }
+
+ static bool isValidIndexForm(unsigned scale, int32_t offset, Optional<Width> width = Nullopt)
+ {
+ if (!isValidScale(scale, width))
+ return false;
+ if (isX86())
+ return true;
+ if (isARM64())
+ return !offset;
+ return false;
+ }
+
+ // If you don't pass a width then this optimistically assumes that you're using the right width. But
+ // the width is relevant to validity, so passing a null width is only useful for assertions. Don't
+ // pass null widths when cascading through Args in the instruction selector!
+ bool isValidForm(Optional<Width> width = Nullopt) const
+ {
+ switch (kind()) {
+ case Invalid:
+ return false;
+ case Tmp:
+ return true;
+ case Imm:
+ return isValidImmForm(value());
+ case Imm64:
+ return true;
+ case Addr:
+ case Stack:
+ case CallArg:
+ return isValidAddrForm(offset());
+ case Index:
+ return isValidIndexForm(offset(), scale(), width);
+ case RelCond:
+ case ResCond:
+ case DoubleCond:
+ case Special:
+ return true;
+ }
+ }
+
template<typename Functor>
void forEachTmpFast(const Functor& functor)
{
Modified: trunk/Source/_javascript_Core/b3/air/opcode_generator.rb (194400 => 194401)
--- trunk/Source/_javascript_Core/b3/air/opcode_generator.rb 2015-12-23 23:45:17 UTC (rev 194400)
+++ trunk/Source/_javascript_Core/b3/air/opcode_generator.rb 2015-12-24 00:14:13 UTC (rev 194401)
@@ -51,6 +51,14 @@
@type = type
@width = width
end
+
+ def widthCode
+ if width == "Ptr"
+ "Arg::pointerWidth()"
+ else
+ "Arg::Width#{width}"
+ end
+ end
end
class Overload
@@ -644,13 +652,7 @@
raise
end
- if arg.width == "Ptr"
- width = "Arg::pointerWidth()"
- else
- width = "Arg::Width#{arg.width}"
- end
-
- outp.puts "functor(args[#{index}], Arg::#{role}, Arg::#{arg.type}P, #{width});"
+ outp.puts "functor(args[#{index}], Arg::#{role}, Arg::#{arg.type}P, #{arg.widthCode});"
}
end
}
@@ -674,9 +676,11 @@
callback = proc {
| form |
notSpecial = (not form.kinds.detect { | kind | kind.special })
- beginArchs(outp, form.archs)
- outp.puts "OPGEN_RETURN(#{notSpecial});"
- endArchs(outp, form.archs)
+ if notSpecial
+ beginArchs(outp, form.archs)
+ outp.puts "OPGEN_RETURN(true);"
+ endArchs(outp, form.archs)
+ end
}
matchForms(outp, :safe, overload.forms, 0, columnGetter, filter, callback)
outp.puts "break;"
@@ -746,17 +750,30 @@
needsMoreValidation = false
overload.signature.length.times {
| index |
- role = overload.signature[index].role
- type = overload.signature[index].type
+ arg = overload.signature[index]
kind = form.kinds[index]
needsMoreValidation |= kind.special
-
- # We already know that the form matches. We don't have to validate the role, since
- # kind implies role. So, the only thing left to validate is the type. And we only have
- # to validate the type if we have a Tmp.
- if kind.name == "Tmp"
- outp.puts "if (!args[#{index}].tmp().is#{type}P())"
+
+ # Some kinds of Args reqire additional validation.
+ case kind.name
+ when "Tmp"
+ outp.puts "if (!args[#{index}].tmp().is#{arg.type}P())"
outp.puts "OPGEN_RETURN(false);"
+ when "Imm"
+ outp.puts "if (!Arg::isValidImmForm(args[#{index}].value()))"
+ outp.puts "OPGEN_RETURN(false);"
+ when "Addr"
+ outp.puts "if (!Arg::isValidAddrForm(args[#{index}].offset()))"
+ outp.puts "OPGEN_RETURN(false);"
+ when "Index"
+ outp.puts "if (!Arg::isValidIndexForm(args[#{index}].scale(), args[#{index}].offset(), #{arg.widthCode}))"
+ outp.puts "OPGEN_RETURN(false);"
+ when "Imm64"
+ when "RelCond"
+ when "ResCond"
+ when "DoubleCond"
+ else
+ raise "Unexpected kind: #{kind.name}"
end
}
if needsMoreValidation
