[Webkit-unassigned] [Bug 162125] Enable IC for put_by_id_with_this

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=162125

Saam Barati <sbarati at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #308823|review?                     |review-
              Flags|                            |

--- Comment #32 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 308823
  --> https://bugs.webkit.org/attachment.cgi?id=308823
proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308823&action=review

Some comments below. Much of this looks good, but I think there are still some bugs to work out.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4795
> +        CallSiteIndex callSite = m_jit.recordCallSiteAndGenerateExceptionHandlingOSRExitIfNeeded(node->origin.semantic, m_stream->size());
> +
> +        JITPutByIdWithThisGenerator gen(
> +            m_jit.codeBlock(), node->origin.semantic, callSite, this->usedRegisters(), JSValueRegs(valueGPR), JSValueRegs(baseGPR),
> +            JSValueRegs(thisGPR), scratchGPR, m_jit.ecmaModeFor(node->origin.semantic));
> +
> +        gen.generateFastPath(m_jit);
> +
> +        JITCompiler::JumpList slowCases;
> +        slowCases.append(gen.slowPathJump());

I think we want to flushRegisters here since the most likely use of this is for set/get. Then, you'd also want to do the thing where you clear base/value/this from the set of usedRegisters.

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:171
> +    JSValueRegs valueRegs, JSValueRegs baseRegs, JSValueRegs thisValueRegs, GPRReg scratch,

What's the reason for this scratch parameter? I don't see a need for it. If all we're doing is clearing something from the used register set, I think this should just fall out naturally.
Is there a reason why this is needed?

> Source/JavaScriptCore/jit/JITOperations.cpp:556
> +void JIT_OPERATION operationPutByIdWithThisNonStrict(ExecState* exec, StructureStubInfo* stubInfo, EncodedJSValue encodedValue, EncodedJSValue encodedBase, EncodedJSValue encodedThis, UniquedStringImpl* uid)

Style Nit: I would add "generic" to this name to match the above function for consistency.

> Source/JavaScriptCore/jit/JITOperations.cpp:625
> +    if (accessType != static_cast<AccessType>(stubInfo->accessType))
> +        return;

I know that the normal put IC path does this, but I don't understand how this could ever be true? I did a quick search, and don't see that this is assigned to after StructureStubInfo's constructor. Am I missing something?
If I'm not missing anything, and this field never gets assigned to besides in the constructor, perhaps it's worth a cleanup either before or after your patch.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:753
> +    // In order to be able to patch both the Structure, and the object offset, we store one pointer,
> +    // to just after the arguments have been loaded into registers 'hotPathBegin', and we generate code
> +    // such that the Structure & offset are always at the same distance from this.

Why is this comment relevant?

> Source/JavaScriptCore/jit/Repatch.cpp:372
> +#if USE(JSVALUE64)

If you're doing this type of thing I'd also make the enumeration value exist on 64-bit platforms.

> Source/JavaScriptCore/jit/Repatch.cpp:491
> +    } else if (slot.base() != baseValue && slot.isCacheablePut() && putKind == WithThis) {

Why do we only do this for WithThis? This seems wrong.

> Source/JavaScriptCore/jit/Repatch.cpp:493
> +            if (structure->isValidOffset(slot.cachedOffset())) {

This seems wrong. Structure is for the base object, not the slot.base(), right?

> Source/JavaScriptCore/jit/Repatch.cpp:498
> +                if (stubInfo.cacheType == CacheType::Unset
> +                    && InlineAccess::canGenerateSelfPropertyReplace(stubInfo, slot.cachedOffset())
> +                    && !structure->needImpurePropertyWatchpoint()
> +                    && !structure->inferredTypeFor(ident.impl())) {

This looks wrong, your above check for `slot.base() != baseValue` means this was not a self property replace.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170509/d769f058/attachment-0001.html>