Attachment 348456 Details for Bug 158911 – patch

[patch] patch

c-backup.diff (text/plain), 14.05 KB, created by Saam Barati on 2018-08-29 18:08:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-08-29 18:08:55 PDT

Size: 14.05 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 235490)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,45 @@
>+2018-08-29  Saam barati  <sbarati@apple.com>
>+
>+        InlineAccess should do StringLength
>+        https://bugs.webkit.org/show_bug.cgi?id=158911
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This patch extends InlineAccess to support StringLength. This patch also
>+        fixes AccessCase::fromStructureStubInfo to support ArrayLength and StringLength.
>+        I forgot to implement this for ArrayLength in the initial InlineAccess
>+        implementation.  Supporting StringLength is a natural extension of the
>+        InlineAccess machinery.
>+
>+        * assembler/MacroAssembler.h:
>+        (JSC::MacroAssembler::patchableBranch8):
>+        * assembler/MacroAssemblerARM64.h:
>+        (JSC::MacroAssemblerARM64::patchableBranch8):
>+        * bytecode/AccessCase.cpp:
>+        (JSC::AccessCase::fromStructureStubInfo):
>+        * bytecode/BytecodeDumper.cpp:
>+        (JSC::BytecodeDumper<Block>::printGetByIdCacheStatus):
>+        * bytecode/InlineAccess.cpp:
>+        (JSC::InlineAccess::dumpCacheSizesAndCrash):
>+        (JSC::InlineAccess::generateSelfPropertyAccess):
>+        (JSC::getScratchRegister):
>+        (JSC::InlineAccess::generateSelfPropertyReplace):
>+        (JSC::InlineAccess::generateArrayLength):
>+        (JSC::InlineAccess::generateSelfInAccess):
>+        (JSC::InlineAccess::generateStringLength):
>+        * bytecode/InlineAccess.h:
>+        * bytecode/PolymorphicAccess.cpp:
>+        (JSC::PolymorphicAccess::regenerate):
>+        * bytecode/StructureStubInfo.cpp:
>+        (JSC::StructureStubInfo::initStringLength):
>+        (JSC::StructureStubInfo::deref):
>+        (JSC::StructureStubInfo::aboutToDie):
>+        (JSC::StructureStubInfo::propagateTransitions):
>+        * bytecode/StructureStubInfo.h:
>+        (JSC::StructureStubInfo::baseGPR const):
>+        * jit/Repatch.cpp:
>+        (JSC::tryCacheGetByID):
>+
> 2018-08-29  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r235432 and r235436.
>Index: Source/JavaScriptCore/assembler/MacroAssembler.h
>===================================================================
>--- Source/JavaScriptCore/assembler/MacroAssembler.h	(revision 235475)
>+++ Source/JavaScriptCore/assembler/MacroAssembler.h	(working copy)
>@@ -450,6 +450,11 @@ public:
>         return PatchableJump(branch32(cond, reg, imm));
>     }
> 
>+    PatchableJump patchableBranch8(RelationalCondition cond, Address address, TrustedImm32 imm)
>+    {
>+        return PatchableJump(branch8(cond, address, imm));
>+    }
>+
>     PatchableJump patchableBranch32(RelationalCondition cond, Address address, TrustedImm32 imm)
>     {
>         return PatchableJump(branch32(cond, address, imm));
>Index: Source/JavaScriptCore/assembler/MacroAssemblerARM64.h
>===================================================================
>--- Source/JavaScriptCore/assembler/MacroAssemblerARM64.h	(revision 235475)
>+++ Source/JavaScriptCore/assembler/MacroAssemblerARM64.h	(working copy)
>@@ -3388,6 +3388,14 @@ public:
>         return PatchableJump(result);
>     }
> 
>+    PatchableJump patchableBranch8(RelationalCondition cond, Address left, TrustedImm32 imm)
>+    {
>+        m_makeJumpPatchable = true;
>+        Jump result = branch8(cond, left, imm);
>+        m_makeJumpPatchable = false;
>+        return PatchableJump(result);
>+    }
>+
>     PatchableJump patchableBranchTest32(ResultCondition cond, RegisterID reg, TrustedImm32 mask = TrustedImm32(-1))
>     {
>         m_makeJumpPatchable = true;
>Index: Source/JavaScriptCore/bytecode/AccessCase.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/AccessCase.cpp	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/AccessCase.cpp	(working copy)
>@@ -121,6 +121,12 @@ std::unique_ptr<AccessCase> AccessCase::
>     case CacheType::InByIdSelf:
>         return AccessCase::create(vm, owner, InHit, stubInfo.u.byIdSelf.offset, stubInfo.u.byIdSelf.baseObjectStructure.get());
> 
>+    case CacheType::ArrayLength:
>+        return AccessCase::create(vm, owner, AccessCase::ArrayLength);
>+
>+    case CacheType::StringLength:
>+        return AccessCase::create(vm, owner, AccessCase::StringLength);
>+
>     default:
>         return nullptr;
>     }
>Index: Source/JavaScriptCore/bytecode/BytecodeDumper.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/BytecodeDumper.cpp	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/BytecodeDumper.cpp	(working copy)
>@@ -447,6 +447,9 @@ void BytecodeDumper<Block>::printGetById
>         case CacheType::ArrayLength:
>             out.printf("ArrayLength");
>             break;
>+        case CacheType::StringLength:
>+            out.printf("StringLength");
>+            break;
>         default:
>             RELEASE_ASSERT_NOT_REACHED();
>             break;
>Index: Source/JavaScriptCore/bytecode/InlineAccess.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/InlineAccess.cpp	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/InlineAccess.cpp	(working copy)
>@@ -47,6 +47,18 @@ void InlineAccess::dumpCacheSizesAndCras
> #else
>     JSValueRegs regs(base);
> #endif
>+    {
>+         CCallHelpers jit;
>+ 
>+        jit.patchableBranch8(
>+            CCallHelpers::NotEqual,
>+            CCallHelpers::Address(base, JSCell::typeInfoTypeOffset()),
>+            CCallHelpers::TrustedImm32(StringType));
>+        jit.load32(CCallHelpers::Address(base, JSString::offsetOfLength()), regs.payloadGPR());
>+        jit.boxInt32(regs.payloadGPR(), regs);
>+
>+        dataLog("string length size: ", jit.m_assembler.buffer().codeSize(), "\n");
>+    }
> 
>     {
>         CCallHelpers jit;
>@@ -158,7 +170,7 @@ bool InlineAccess::generateSelfPropertyA
> {
>     CCallHelpers jit;
>     
>-    GPRReg base = static_cast<GPRReg>(stubInfo.patch.baseGPR);
>+    GPRReg base = stubInfo.baseGPR();
>     JSValueRegs value = stubInfo.valueRegs();
> 
>     auto branchToSlowPath = jit.patchableBranch32(
>@@ -185,7 +197,7 @@ bool InlineAccess::generateSelfPropertyA
> ALWAYS_INLINE static GPRReg getScratchRegister(StructureStubInfo& stubInfo)
> {
>     ScratchRegisterAllocator allocator(stubInfo.patch.usedRegisters);
>-    allocator.lock(static_cast<GPRReg>(stubInfo.patch.baseGPR));
>+    allocator.lock(stubInfo.baseGPR());
>     allocator.lock(static_cast<GPRReg>(stubInfo.patch.valueGPR));
> #if USE(JSVALUE32_64)
>     allocator.lock(static_cast<GPRReg>(stubInfo.patch.baseTagGPR));
>@@ -216,7 +228,7 @@ bool InlineAccess::generateSelfPropertyR
> 
>     CCallHelpers jit;
> 
>-    GPRReg base = static_cast<GPRReg>(stubInfo.patch.baseGPR);
>+    GPRReg base = stubInfo.baseGPR();
>     JSValueRegs value = stubInfo.valueRegs();
> 
>     auto branchToSlowPath = jit.patchableBranch32(
>@@ -258,7 +270,7 @@ bool InlineAccess::generateArrayLength(S
> 
>     CCallHelpers jit;
> 
>-    GPRReg base = static_cast<GPRReg>(stubInfo.patch.baseGPR);
>+    GPRReg base = stubInfo.baseGPR();
>     JSValueRegs value = stubInfo.valueRegs();
>     GPRReg scratch = getScratchRegister(stubInfo);
> 
>@@ -276,11 +288,32 @@ bool InlineAccess::generateArrayLength(S
>     return linkedCodeInline;
> }
> 
>+bool InlineAccess::generateStringLength(StructureStubInfo& stubInfo)
>+{
>+    CCallHelpers jit;
>+
>+    GPRReg base = stubInfo.baseGPR();
>+    JSValueRegs value = stubInfo.valueRegs();
>+
>+    auto branchToSlowPath = jit.patchableBranch8(
>+        CCallHelpers::NotEqual,
>+        CCallHelpers::Address(base, JSCell::typeInfoTypeOffset()),
>+        CCallHelpers::TrustedImm32(StringType));
>+    jit.load32(CCallHelpers::Address(base, JSString::offsetOfLength()), value.payloadGPR());
>+    jit.boxInt32(value.payloadGPR(), value);
>+
>+    bool linkedCodeInline = linkCodeInline("string length", jit, stubInfo, [&] (LinkBuffer& linkBuffer) {
>+        linkBuffer.link(branchToSlowPath, stubInfo.slowPathStartLocation());
>+    });
>+    return linkedCodeInline;
>+}
>+
>+
> bool InlineAccess::generateSelfInAccess(StructureStubInfo& stubInfo, Structure* structure)
> {
>     CCallHelpers jit;
> 
>-    GPRReg base = static_cast<GPRReg>(stubInfo.patch.baseGPR);
>+    GPRReg base = stubInfo.baseGPR();
>     JSValueRegs value = stubInfo.valueRegs();
> 
>     auto branchToSlowPath = jit.patchableBranch32(
>Index: Source/JavaScriptCore/bytecode/InlineAccess.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/InlineAccess.h	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/InlineAccess.h	(working copy)
>@@ -87,7 +87,7 @@ public:
>     // FIXME: Make this constexpr when GCC is able to compile std::max() inside a constexpr function.
>     // https://bugs.webkit.org/show_bug.cgi?id=159436
>     //
>-    // This is the maximum between the size for array length access, and the size for regular self access.
>+    // This is the maximum between array length, string length, and regular self access sizes.
>     ALWAYS_INLINE static size_t sizeForLengthAccess()
>     {
> #if CPU(X86_64)
>@@ -117,6 +117,7 @@ public:
>     static bool generateArrayLength(StructureStubInfo&, JSArray*);
>     static void rewireStubAsJump(StructureStubInfo&, CodeLocationLabel<JITStubRoutinePtrTag>);
>     static bool generateSelfInAccess(StructureStubInfo&, Structure*);
>+    static bool generateStringLength(StructureStubInfo&);
> 
>     // This is helpful when determining the size of an IC on
>     // various platforms. When adding a new type of IC, implement
>Index: Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp	(working copy)
>@@ -381,7 +381,7 @@ AccessGenerationResult PolymorphicAccess
>     state.stubInfo = &stubInfo;
>     state.ident = &ident;
>     
>-    state.baseGPR = static_cast<GPRReg>(stubInfo.patch.baseGPR);
>+    state.baseGPR = stubInfo.baseGPR();
>     state.thisGPR = static_cast<GPRReg>(stubInfo.patch.thisGPR);
>     state.valueRegs = stubInfo.valueRegs();
> 
>Index: Source/JavaScriptCore/bytecode/StructureStubInfo.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/StructureStubInfo.cpp	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/StructureStubInfo.cpp	(working copy)
>@@ -73,6 +73,11 @@ void StructureStubInfo::initArrayLength(
>     cacheType = CacheType::ArrayLength;
> }
> 
>+void StructureStubInfo::initStringLength()
>+{
>+    cacheType = CacheType::StringLength;
>+}
>+
> void StructureStubInfo::initPutByIdReplace(CodeBlock* codeBlock, Structure* baseObjectStructure, PropertyOffset offset)
> {
>     cacheType = CacheType::PutByIdReplace;
>@@ -102,6 +107,7 @@ void StructureStubInfo::deref()
>     case CacheType::PutByIdReplace:
>     case CacheType::InByIdSelf:
>     case CacheType::ArrayLength:
>+    case CacheType::StringLength:
>         return;
>     }
> 
>@@ -119,6 +125,7 @@ void StructureStubInfo::aboutToDie()
>     case CacheType::PutByIdReplace:
>     case CacheType::InByIdSelf:
>     case CacheType::ArrayLength:
>+    case CacheType::StringLength:
>         return;
>     }
> 
>@@ -292,6 +299,7 @@ bool StructureStubInfo::propagateTransit
>     switch (cacheType) {
>     case CacheType::Unset:
>     case CacheType::ArrayLength:
>+    case CacheType::StringLength:
>         return true;
>     case CacheType::GetByIdSelf:
>     case CacheType::PutByIdReplace:
>Index: Source/JavaScriptCore/bytecode/StructureStubInfo.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/StructureStubInfo.h	(revision 235475)
>+++ Source/JavaScriptCore/bytecode/StructureStubInfo.h	(working copy)
>@@ -61,7 +61,8 @@ enum class CacheType : int8_t {
>     PutByIdReplace,
>     InByIdSelf,
>     Stub,
>-    ArrayLength
>+    ArrayLength,
>+    StringLength
> };
> 
> class StructureStubInfo {
>@@ -73,6 +74,7 @@ public:
> 
>     void initGetByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
>     void initArrayLength();
>+    void initStringLength();
>     void initPutByIdReplace(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
>     void initInByIdSelf(CodeBlock*, Structure* baseObjectStructure, PropertyOffset);
> 
>@@ -199,6 +201,11 @@ public:
> #endif
>     } patch;
> 
>+    GPRReg baseGPR() const
>+    {
>+        return static_cast<GPRReg>(patch.baseGPR);
>+    }
>+
>     CodeLocationCall<JSInternalPtrTag> slowPathCallLocation() { return patch.start.callAtOffset<JSInternalPtrTag>(patch.deltaFromStartToSlowPathCallLocation); }
>     CodeLocationLabel<JSInternalPtrTag> doneLocation() { return patch.start.labelAtOffset<JSInternalPtrTag>(patch.inlineSize); }
>     CodeLocationLabel<JITStubRoutinePtrTag> slowPathStartLocation() { return patch.start.labelAtOffset(patch.deltaFromStartToSlowPathStart); }
>Index: Source/JavaScriptCore/jit/Repatch.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/Repatch.cpp	(revision 235475)
>+++ Source/JavaScriptCore/jit/Repatch.cpp	(working copy)
>@@ -215,8 +215,18 @@ static InlineCacheAction tryCacheGetByID
>                 }
> 
>                 newCase = AccessCase::create(vm, codeBlock, AccessCase::ArrayLength);
>-            } else if (isJSString(baseCell))
>+            } else if (isJSString(baseCell)) {
>+                if (stubInfo.cacheType == CacheType::Unset) {
>+                    bool generatedCodeInline = InlineAccess::generateStringLength(stubInfo);
>+                    if (generatedCodeInline) {
>+                        ftlThunkAwareRepatchCall(codeBlock, stubInfo.slowPathCallLocation(), appropriateOptimizingGetByIdFunction(kind));
>+                        stubInfo.initStringLength();
>+                        return RetryCacheLater;
>+                    }
>+                }
>+
>                 newCase = AccessCase::create(vm, codeBlock, AccessCase::StringLength);
>+            }
>             else if (DirectArguments* arguments = jsDynamicCast<DirectArguments*>(vm, baseCell)) {
>                 // If there were overrides, then we can handle this as a normal property load! Guarding
>                 // this with such a check enables us to add an IC case for that load if needed.

Actions: View | Formatted Diff | Diff

Attachments on bug 158911: 281668 | 348456 | 348460