Attachment 362086 Details for Bug 194656 – patch

[patch] patch

a-backup.diff (text/plain), 5.09 KB, created by Saam Barati on 2019-02-14 17:41:53 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-02-14 17:41:53 PST

Size: 5.09 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241573)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,20 @@
>+2019-02-14  Saam Barati  <sbarati@apple.com>
>+
>+        lowerStackArgs should lower Lea32/64 on ARM64
>+        https://bugs.webkit.org/show_bug.cgi?id=194656
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        On arm64, Lea is just implemented as an add. However, Air treats it as an
>+        address with a given width. Because of this width, we were incorrectly
>+        computing whether or not this immediate could fit into the instruction itself
>+        or it needed to be explicitly put into a register. This patch makes
>+        AirLowerStackArgs lower Lea to Add on arm64.
>+
>+        * b3/air/AirLowerStackArgs.cpp:
>+        (JSC::B3::Air::lowerStackArgs):
>+        * b3/air/testair.cpp:
>+
> 2019-02-14  Saam Barati  <sbarati@apple.com>
> 
>         Cache the results of BytecodeGenerator::getVariablesUnderTDZ
>Index: Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp
>===================================================================
>--- Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp	(revision 241573)
>+++ Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp	(working copy)
>@@ -71,6 +71,46 @@ void lowerStackArgs(Code& code)
>         for (unsigned instIndex = 0; instIndex < block->size(); ++instIndex) {
>             Inst& inst = block->at(instIndex);
> 
>+            if (isARM64() && (inst.kind.opcode == Lea32 || inst.kind.opcode == Lea64)) {
>+                // On ARM64, Lea is just an add. We can't handle this below because
>+                // taking into account the Width to see if we can compute the immediate
>+                // is wrong.
>+                auto lowerArmLea = [&] (Value::OffsetType offset, Tmp base) {
>+                    ASSERT(inst.args[1].isTmp());
>+
>+                    if (Arg::isValidImmForm(offset))
>+                        inst = Inst(inst.kind.opcode == Lea32 ? Add32 : Add64, inst.origin, Arg::imm(offset), base, inst.args[1]);
>+                    else {
>+                        ASSERT(pinnedExtendedOffsetAddrRegister());
>+                        Air::Tmp tmp = Air::Tmp(*pinnedExtendedOffsetAddrRegister());
>+                        Arg offsetArg = Arg::bigImm(offset);
>+                        insertionSet.insert(instIndex, Move, inst.origin, offsetArg, tmp);
>+                        inst = Inst(inst.kind.opcode == Lea32 ? Add32 : Add64, inst.origin, tmp, base, inst.args[1]);
>+                    }
>+                };
>+
>+                switch (inst.args[0].kind()) {
>+                case Arg::Stack: {
>+                    StackSlot* slot = inst.args[0].stackSlot();
>+                    lowerArmLea(inst.args[0].offset() + slot->offsetFromFP(), Tmp(GPRInfo::callFrameRegister));
>+                    break;
>+                }
>+                case Arg::CallArg:
>+                    lowerArmLea(inst.args[0].offset() - code.frameSize(), Tmp(GPRInfo::callFrameRegister));
>+                    break;
>+                case Arg::Addr:
>+                    lowerArmLea(inst.args[0].offset(), inst.args[0].base());
>+                    break;
>+                case Arg::ExtendedOffsetAddr:
>+                    ASSERT_NOT_REACHED();
>+                    break;
>+                default:
>+                    break;
>+                }
>+
>+                continue;
>+            }
>+
>             inst.forEachArg(
>                 [&] (Arg& arg, Arg::Role role, Bank, Width width) {
>                     auto stackAddr = [&] (Value::OffsetType offsetFromFP) -> Arg {
>Index: Source/JavaScriptCore/b3/air/testair.cpp
>===================================================================
>--- Source/JavaScriptCore/b3/air/testair.cpp	(revision 241573)
>+++ Source/JavaScriptCore/b3/air/testair.cpp	(working copy)
>@@ -2028,6 +2028,40 @@ void testArgumentRegPinned3()
>     CHECK(r == 10 + 42 + 42);
> }
> 
>+void testLea64()
>+{
>+    B3::Procedure proc;
>+    Code& code = proc.code();
>+
>+    BasicBlock* root = code.addBlock();
>+
>+    int64_t a = 0x11223344;
>+    int64_t b = 1 << 13;
>+
>+    root->append(Lea64, nullptr, Arg::addr(Tmp(GPRInfo::argumentGPR0), b), Tmp(GPRInfo::returnValueGPR));
>+    root->append(Ret64, nullptr, Tmp(GPRInfo::returnValueGPR));
>+
>+    int64_t r = compileAndRun<int64_t>(proc, a);
>+    CHECK(r == a + b);
>+}
>+
>+void testLea32()
>+{
>+    B3::Procedure proc;
>+    Code& code = proc.code();
>+
>+    BasicBlock* root = code.addBlock();
>+
>+    int32_t a = 0x11223344;
>+    int32_t b = 1 << 13;
>+
>+    root->append(Lea32, nullptr, Arg::addr(Tmp(GPRInfo::argumentGPR0), b), Tmp(GPRInfo::returnValueGPR));
>+    root->append(Ret32, nullptr, Tmp(GPRInfo::returnValueGPR));
>+
>+    int32_t r = compileAndRun<int32_t>(proc, a);
>+    CHECK(r == a + b);
>+}
>+
> #define RUN(test) do {                          \
>         if (!shouldRun(#test))                  \
>             break;                              \
>@@ -2113,6 +2147,9 @@ void run(const char* filter)
>     RUN(testArgumentRegPinned2());
>     RUN(testArgumentRegPinned3());
> 
>+    RUN(testLea32());
>+    RUN(testLea64());
>+
>     if (tasks.isEmpty())
>         usage();
>

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194656: 362031 | 362070 | 362086