LLInt get_by_id unset IC looks inadventently disabled

[Angelos Oikonomopoulos]

LLInt get_by_id IC looks inadventently disabled

[Angelos Oikonomopoulos]

Created attachment 434424 [details]
Patch

[Angelos Oikonomopoulos]

This commit

https://github.com/WebKit/WebKit/commit/22d3ec3ad0249d03d9a909c909fb1ccc246e1de5#diff-95ecbe05ccde2fa9490e1bac06bc5a159fabc13dc58aa4e7a33114fd95176d0aR775

appears to accidentally disable unsetMode for the IC in performLLIntGetByID. Specifically, we can only switch to unsetMode in setupGetByIdPrototypeCache, but the only call to setupGetByIdPrototypeCache is guarded by

     if (!LLINT_ALWAYS_ACCESS_SLOW
         && baseValue.isCell()
         && slot.isCacheable()
         && !slot.isUnset()) {

in the body of performLLIntGetByID.

[Saam Barati]

(In reply to Angelos Oikonomopoulos from comment #0)
> LLInt get_by_id IC looks inadventently disabled

The name of this bug seems off. I think you mean just proto loads.

[Angelos Oikonomopoulos]

(In reply to Saam Barati from comment #3)
> (In reply to Angelos Oikonomopoulos from comment #0)
> > LLInt get_by_id IC looks inadventently disabled
> 
> The name of this bug seems off. I think you mean just proto loads.

I meant unsetMode. Updated the name.

[Saam Barati]

Comment on attachment 434424 [details]
Patch

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775
> -        && slot.isCacheable()
> -        && !slot.isUnset()) {
> +        && slot.isCacheable()) {

I don't see how this is doing anything. In the below code, we only check for isValue before doing proto loads. What am I missing?

[Angelos Oikonomopoulos]

(In reply to Saam Barati from comment #5)
> Comment on attachment 434424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434424&action=review
> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775
> > -        && slot.isCacheable()
> > -        && !slot.isUnset()) {
> > +        && slot.isCacheable()) {
> 
> I don't see how this is doing anything. In the below code, we only check for
> isValue before doing proto loads. What am I missing?

Going by your use of 'only', what you're missing may be that

} else if (UNLIKELY(metadata.hitCountForLLIntCaching && slot.isValue())) {

is nested within

if (!LLINT_ALWAYS_ACCESS_SLOW
        && baseValue.isCell()
        && slot.isCacheable()
        && !slot.isUnset()) {

?

[Saam Barati]

(In reply to Angelos Oikonomopoulos from comment #6)
> (In reply to Saam Barati from comment #5)
> > Comment on attachment 434424 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=434424&action=review
> > 
> > > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:775
> > > -        && slot.isCacheable()
> > > -        && !slot.isUnset()) {
> > > +        && slot.isCacheable()) {
> > 
> > I don't see how this is doing anything. In the below code, we only check for
> > isValue before doing proto loads. What am I missing?
> 
> Going by your use of 'only', what you're missing may be that
> 
> } else if (UNLIKELY(metadata.hitCountForLLIntCaching && slot.isValue())) {
> 
> is nested within
> 
> if (!LLINT_ALWAYS_ACCESS_SLOW
>         && baseValue.isCell()
>         && slot.isCacheable()
>         && !slot.isUnset()) {
> 
> ?

slot.isValue() implies !slot.isUnset(). You can't be both a value and unset. So I don't see how removing !slot.isUnset() changes anything to be different in the proto load case, since we check slot.isValue()

[Angelos Oikonomopoulos]

Fair enough, the patch is a no-op then. But AFAICT unsetMode remains effectively disabled, since the only call to setupGetByIdPrototypeCache is guarded by the slot.isValue() check.

[Saam Barati]

(In reply to Angelos Oikonomopoulos from comment #8)
> Fair enough, the patch is a no-op then. But AFAICT unsetMode remains
> effectively disabled, since the only call to setupGetByIdPrototypeCache is
> guarded by the slot.isValue() check.

Right. Maybe that call to setupGetByIdPrototypeCache shouldn't be guarded by isValue()?

[Saam Barati]

Clearing "r?" for now

[Radar WebKit Bug Importer]

<rdar://problem/81511696>