[webkit-reviews] review denied: [Bug 162125] Enable IC for put_by_id_with_this : [Attachment 308823] proposed Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Caio Lima <ticaiolima at gmail.com>'s
request for review:
Bug 162125: Enable IC for put_by_id_with_this
https://bugs.webkit.org/show_bug.cgi?id=162125

Attachment 308823: proposed Patch

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




--- 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.se
mantic, 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.