Bug 162125 - Enable IC for put_by_id_with_this
Summary: Enable IC for put_by_id_with_this
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 157215
  Show dependency treegraph
 
Reported: 2016-09-17 18:41 PDT by Caio Lima
Modified: 2017-05-09 13:22 PDT (History)
7 users (show)

See Also:


Attachments
WIP - Proposed Patch (48.53 KB, patch)
2017-03-26 10:23 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (984.70 KB, application/zip)
2017-03-27 07:44 PDT, Build Bot
no flags Details
WIP - Proposed Patch (54.75 KB, patch)
2017-03-27 09:01 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP - Benchmarks (85.44 KB, text/plain)
2017-03-27 09:08 PDT, Caio Lima
no flags Details
Patch (61.20 KB, patch)
2017-03-27 19:08 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (57.87 KB, patch)
2017-03-30 19:47 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Benchmark (84.14 KB, text/plain)
2017-03-30 19:52 PDT, Caio Lima
no flags Details
WIP - Patch (56.95 KB, patch)
2017-04-05 09:15 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
Proposed Patch (68.95 KB, patch)
2017-04-25 18:43 PDT, Caio Lima
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.09 MB, application/zip)
2017-04-25 20:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (7.63 MB, application/zip)
2017-04-25 21:03 PDT, Build Bot
no flags Details
Proposed Patch (68.97 KB, patch)
2017-04-26 07:38 PDT, Caio Lima
sbarati: review-
Details | Formatted Diff | Diff
Proposed Patch (65.92 KB, patch)
2017-04-26 20:40 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
proposed Patch (65.98 KB, patch)
2017-05-02 07:58 PDT, Caio Lima
sbarati: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Lima 2016-09-17 18:41:11 PDT
The current implementation is using the instruction op_put_by_id_with_this to handle super setter bindings correctly. This operand is just a call to slow path which means that setters called from super aren't optimized to use IC or accessor inlining.

To unify these opcodes, we need to add one more operand on op_put_by_id to store the this reference and teach the following layers and ICs that base and this operands sometimes aren't the same.
Comment 1 Caio Lima 2017-03-26 10:23:56 PDT
Created attachment 305425 [details]
WIP - Proposed Patch

It is a WIP for this Patch. I'm missing the ChangeLog and create some micro benchmarks as well.

In this patch I'm not enabling PutByIdWithThis IC for 32-bit architectures, because there is no sufficient GPRRegs to handle base, this and value and scratch at the same time.
Comment 2 Build Bot 2017-03-26 10:27:06 PDT
Attachment 305425 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9907:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4564:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2017-03-27 07:44:00 PDT
Comment on attachment 305425 [details]
WIP - Proposed Patch

Attachment 305425 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3417323

New failing tests:
fast/css/getComputedStyle/computed-style-font-family.html
Comment 4 Build Bot 2017-03-27 07:44:02 PDT
Created attachment 305466 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 5 Caio Lima 2017-03-27 09:01:25 PDT
Created attachment 305470 [details]
WIP - Proposed Patch

I noticed that IC was not being properly handled in put_by_id_with_this in the last patch because slot object wasn't properly configured in ```ordinarySetSlow```. Now in this version I'm proposing a change that configure the slot properly and make possible IC PuByIdWithThis.

The results seem promising and I'm posting them here soon.
Comment 6 Build Bot 2017-03-27 09:04:11 PDT
Attachment 305470 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1541:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9907:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4564:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 5 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Caio Lima 2017-03-27 09:08:02 PDT
Created attachment 305471 [details]
WIP - Benchmarks

This is the last run I've made before the last Patch. I still need to update it using the last Patch uploaded.It is ~6x faster into a new micro benchmark that I've written named "super-setter". However, there are some regressions that I don't like. Investigating how to fix them.
Comment 8 Caio Lima 2017-03-27 19:08:43 PDT
Created attachment 305541 [details]
Patch
Comment 9 Build Bot 2017-03-28 00:44:29 PDT
Attachment 305541 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1541:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Caio Lima 2017-03-30 19:47:50 PDT
Created attachment 305938 [details]
Proposed Patch

This version is fixing the previous case. In the previous case the IC was not being enabled when the property wasn't accessor.
Comment 11 Build Bot 2017-03-30 19:50:21 PDT
Attachment 305938 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Caio Lima 2017-03-30 19:52:45 PDT
Created attachment 305939 [details]
Benchmark

This is the new Benchmark from last Patch version. Finally the overall result is neutral and there is no regression into micro benchmarks with good win into put-by-id-with-this cases.

   super-get-by-id-with-this-monomorphic ^ definitely 1.1759x faster
   super-get-by-id-with-this-polymorphic ^ definitely 1.2914x faster
   super-set-value                       ^ definitely 7.4017x faster
   super-setter                          ^ definitely 6.1591x faster
Comment 13 Caio Lima 2017-04-03 20:52:13 PDT
Ping review
Comment 14 Saam Barati 2017-04-04 15:58:09 PDT
Comment on attachment 305938 [details]
Proposed Patch

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

mostly LGTM, just a couple comments. I also think I found a bug.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1912
> +#if USE(JSVALUE64_32)
> +        case PutByIdWithThis:
> +#endif

This is wrong. I believe it should be JSVALUE32_64. I think it's simpler if you just leave the 32-bit version in the code above, and have the USE(JSVALUE64) over a smaller range.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4591
> +        JITCompiler::JumpList notCellList;
> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(baseGPR)));
> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisGPR)));

Looks like a bug that you don't use this after appending to it. Please add tests. Should be an Insta crash.

That said, the more I think about it, are there any scenarios where |this|/base won't be a cell?

> Source/JavaScriptCore/jit/Repatch.cpp:353
> +        if (UNLIKELY(putKind == WithThis))

I don't think you need the UNLIKELY here. It might be the case that users do use this a lot.
Comment 15 Caio Lima 2017-04-05 08:59:48 PDT
Comment on attachment 305938 [details]
Proposed Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4591
>> +        notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisGPR)));
> 
> Looks like a bug that you don't use this after appending to it. Please add tests. Should be an Insta crash.
> 
> That said, the more I think about it, are there any scenarios where |this|/base won't be a cell?

Oops. It should be a cachedPutByIdWithThis parameter. However, I'm following your thinking too. __proto__ can be "null" or  "object". When it is null, ```super.something``` will fail as TypeError. (https://tc39.github.io/ecma262/#sec-set-object.prototype.__proto__). If I'm not missing something, either base/this are always Cell.

>> Source/JavaScriptCore/jit/Repatch.cpp:353
>> +        if (UNLIKELY(putKind == WithThis))
> 
> I don't think you need the UNLIKELY here. It might be the case that users do use this a lot.

Done.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:205
> +    if (UNLIKELY(isThisValueAltered(slot, thisObject) && slot.context() != PutPropertySlot::PutByIdWithThis))

Just got a case where this condition is not ok. Investigating a properly way to set slot properly here.
Comment 16 Caio Lima 2017-04-05 09:15:35 PDT
Created attachment 306287 [details]
WIP - Patch

Just found a bug in this approach to enable IC. Sending this to see the tests.

A new Patch is coming soon.
Comment 17 Build Bot 2017-04-05 09:18:13 PDT
Attachment 306287 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Caio Lima 2017-04-25 18:43:24 PDT
Created attachment 308185 [details]
Proposed Patch

This Version is enabling put_by_id_with_this for setter and value cases. Here are some benchmarks improved with this patch:

super-set-value                       ^ definitely 9.2902x faster
super-setter                          ^ definitely 5.9915x faster
untyped-string-from-char-code         ^ definitely 1.0725x faster
super-get-by-id-with-this-monomorphic ^ definitely 1.1543x faster
Comment 19 Build Bot 2017-04-25 18:45:28 PDT
Attachment 308185 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Build Bot 2017-04-25 20:15:00 PDT
Comment on attachment 308185 [details]
Proposed Patch

Attachment 308185 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3606647

New failing tests:
webrtc/datachannel/basic.html
Comment 21 Build Bot 2017-04-25 20:15:01 PDT
Created attachment 308196 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 22 Build Bot 2017-04-25 21:03:50 PDT
Comment on attachment 308185 [details]
Proposed Patch

Attachment 308185 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3606700

New failing tests:
fast/forms/file/intrinsic-min-width-overrides-width.html
Comment 23 Build Bot 2017-04-25 21:03:52 PDT
Created attachment 308207 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 24 Caio Lima 2017-04-26 07:38:09 PDT
Created attachment 308242 [details]
Proposed Patch

Fixing some NITs, rebased with upstream code and re-triggering EWS.
Comment 25 Build Bot 2017-04-26 07:40:13 PDT
Attachment 308242 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 Saam Barati 2017-04-26 13:07:26 PDT
Comment on attachment 308242 [details]
Proposed Patch

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

I'll keep looking at the rest of the patch, but some basic comments for now.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1926
> +#if USE(JSVALUE64)
> +        {
> +            fixEdge<CellUse>(node->child1());
> +            fixEdge<CellUse>(node->child2());
> +            break;
> +        }
> +#endif

Style: This seems slightly too tricky. I'd just have this be its own case, with a "break" both for 64/32 bit platforms.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:249
> +void SpeculativeJIT::cachedPutByIdWithThis(CodeOrigin codeOrigin, GPRReg baseGPR, GPRReg valueGPR, GPRReg thisGPR, GPRReg scratchGPR, unsigned identifierNumber, JITCompiler::JumpList slowPathTargets, SpillRegistersMode spillMode)

There is only one caller here, which never provides spillmode, please remove the parameter. Also, since this is a function w/ a single caller, I'd just remove this function alltogether and just inline it at the only call site.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2944
> +        if (m_node->child1().useKind() == CellUse && m_node->child2().useKind() == CellUse)
> +            putByIdWithThis(lowJSValue(m_node->child3()), lowCell(m_node->child1()), lowCell(m_node->child2()));

You unconditionally speculate CellUse in FixupPhase, but then check here if it's not CellUse. Perhaps FixupPhase's speculation should be conditional? Otherwise, that "else" branch is most likely dead code.
Comment 27 Caio Lima 2017-04-26 20:40:13 PDT
Created attachment 308330 [details]
Proposed Patch

Addressing Saam's review
Comment 28 Build Bot 2017-04-26 20:43:10 PDT
Attachment 308330 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Caio Lima 2017-04-27 07:58:25 PDT
Comment on attachment 308242 [details]
Proposed Patch

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

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1926
>> +#endif
> 
> Style: This seems slightly too tricky. I'd just have this be its own case, with a "break" both for 64/32 bit platforms.

Done.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:249
>> +void SpeculativeJIT::cachedPutByIdWithThis(CodeOrigin codeOrigin, GPRReg baseGPR, GPRReg valueGPR, GPRReg thisGPR, GPRReg scratchGPR, unsigned identifierNumber, JITCompiler::JumpList slowPathTargets, SpillRegistersMode spillMode)
> 
> There is only one caller here, which never provides spillmode, please remove the parameter. Also, since this is a function w/ a single caller, I'd just remove this function alltogether and just inline it at the only call site.

Sounds better.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2944
>> +            putByIdWithThis(lowJSValue(m_node->child3()), lowCell(m_node->child1()), lowCell(m_node->child2()));
> 
> You unconditionally speculate CellUse in FixupPhase, but then check here if it's not CellUse. Perhaps FixupPhase's speculation should be conditional? Otherwise, that "else" branch is most likely dead code.

Oops. Removed from DFG and forgot it here.
Comment 30 Caio Lima 2017-05-02 07:58:17 PDT
Created attachment 308823 [details]
proposed Patch

Rebasing with master and ping for review.
Comment 31 Build Bot 2017-05-02 08:00:37 PDT
Attachment 308823 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:1544:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144:  The parameter name "valueRegs" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Saam Barati 2017-05-09 13:22:31 PDT
Comment on attachment 308823 [details]
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.