Bug 157215 - Unite *with_this opcodes with their non this version
Summary: Unite *with_this opcodes with their non this version
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on: 162125 162126 162127 162124
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-29 16:49 PDT by Saam Barati
Modified: 2016-09-17 18:51 PDT (History)
10 users (show)

See Also:


Attachments
WIP get_by_id (8.41 KB, patch)
2016-08-29 00:04 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP get_by_id PIC fixing (20.34 KB, patch)
2016-09-01 09:01 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP get_by_id in DFG (46.74 KB, patch)
2016-09-03 13:46 PDT, Caio Lima
no flags Details | Formatted Diff | Diff
WIP 64bits support in all layers (deleted)
2016-09-14 23:51 PDT, Caio Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-04-29 16:49:54 PDT
This would probably require making *by_id and *by_val also take a this parameter.
It would also requiring changing ICs.
Comment 1 Caio Lima 2016-08-29 00:04:08 PDT
Created attachment 287252 [details]
WIP get_by_id

This is just the first move on this Patch. I am planing separate into in 2 Patches one to merge get_by_id* and other to put_by_id*

In this version I am justing supporting the LLInt layer.

In LLint layer, I am considering that the property cache can happen as the "ident" being cached as prototype cache of thisValue. It is not enabled to Getters yet, but there is a Patch implementing this (https://bugs.webkit.org/show_bug.cgi?id=158083). Is this reasoning consistent?

My major doubt in Baseline is about PIC. Do we support it to Getter/Setters? If yes, how this mechanism work? If I am not wrong, usually the IC is a stub considering some observed types and loading the correspondent offset for each object, however getter e setter requires a function call, right?
Comment 2 Caio Lima 2016-09-01 09:01:00 PDT
Created attachment 287617 [details]
WIP get_by_id PIC fixing

This version is just a prototype to enable the new version of get_by_id PIC support.

I am not going to keep the different methods like "operationGetByIdOptimizeWithThis" and the JITGetByIdGenerator new constructor in the final version, but I am doing this way because it is easier to debug without change all 4 layers at once.

I am justing enabling it to 64 bits version, but 32 bits support is coming soon.

As I mentioned before, LLInt change isn't adding much because we aren't inline caching accessors yet. However, I noticed that this update enable PIC in Baseline layer.
Comment 3 Caio Lima 2016-09-03 13:46:11 PDT
Created attachment 287868 [details]
WIP get_by_id in DFG

This version is enabling the main cases of get_by_id in DFG layer. I added thisOperand as a child2() of GetById(Flush) nodes. When these nodes are compiled and the getter is inlined, we can set thisOperand as the this of inlined Frame.

What I am missing to finish get_by_id_merge is:

- Merge it in FTL layer;
- Check other cases with Polymorphic and Megamorphic calls;
- Merge get_by_id in x86_32; 

I am having some troubles to compile the following sample in FTL mode:

```
"use strict";
var Base = class Base {
    constructor() { this._name = "Name"; }
    get name() { return this._name; } // If this instead returns a static: return "Foo" things somewhat work.
    set name(x) { this._name = x; }
};

var Subclass = class Subclass extends Base {
    get name() { return super.name; }
};

function getterName(instance) {
    return instance.name;
}

var instance = new Subclass;
for (let i = 0; i < 1000000;i++)
    getterName(instance);
```
I have no idea yet, because I dumped the DFG and found CheckTierUpAtReturn(MustGen, W:SideState, Exits, bc#16). Going to investigate it more.
Comment 4 Caio Lima 2016-09-14 23:51:09 PDT
Created attachment 288932 [details]
WIP 64bits support in all layers

This Patch is implementing the 64bit version of get_by_id merge. The idea is to put this as get_by_id operand and propagate it through Baseline, DFG and FTL. To avoid register usage when this == base, we check this case on  DFG and FTL.

The test is considering Polymorphic and Megamorphic IC. Also there are 2 conditions to be checked that is when --useAccessInline=(false|true). I would like to know if there is a way to set a JSC option from a test script, because this way I can improve the test to consider --useAccessInline=(false|true) directly from tests.

There are a lot of work to do yet and definitely I am going to split the *by_id in 4 patches.