Bug 84324 – DFG can have the spill format identical to the register format

• Status: NEW
• Component: JavaScriptCore

[Yuqiang Xian]

Actually we almost support that except for some minor things. With those fixed the virtual register spilling could be cleaner.

[Yuqiang Xian]

Created attachment 137872 [details]
patch

Also, take the chance to make the filling cleaner especially for the 64bit.

[Yuqiang Xian]

Created attachment 137873 [details]
Performance result

This is performance neutral.

[Yuqiang Xian]

Created attachment 145923 [details]
Rebase

[Filip Pizlo]

Comment on attachment 145923 [details]
Rebase

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

I'm fine with this on principle, but I'm not sure about the calls to terminateSpeculativeExecution().  These days DFG::AbstractState must know exactly where OSR exits may happen since this information is consumed by store elimination.  Hence, any calls to speculationCheck() and terminateSpeculativeExecution() ought to be guarded by checks against things that AbstractState would know during the CFA phase, to ensure that we don't end up performing store elimination under the assumption that there is no OSR exit only to have an OSR exit inserted by the backend.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1043
> +    if (isKnownNotInteger(nodeIndex)) {
> +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> +        returnFormat = DataFormatInteger;
> +        return allocate();
> +    }
> +

Perhaps it's better to use isInt32Prediction(m_state.forNode(nodeIndex).m_type) here?

[Yuqiang Xian]

Created attachment 145924 [details]
o... the previous one is wrongly rebased.. this one please

[Filip Pizlo]

Comment on attachment 145924 [details]
o... the previous one is wrongly rebased.. this one please

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

I think that the code is probably right, in the sense that isKnownNotFooBar() will only return true if the CFA would have also concluded that the code will OSR exit.  But maybe we can put some tighter assertions about this into the code?

Also, the isKnownNotFooBar() and isKnownFooBar() methods are kind of a hack that is left over from the old DFG days.  It would be nice to get rid of them...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1042
> +    if (isKnownNotInteger(nodeIndex)) {
> +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> +        returnFormat = DataFormatInteger;
> +        return allocate();
> +    }

Same comment as before...

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1175
> +    if (isKnownNotNumber(nodeIndex)) {
> +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> +        return fprAllocate();
> +    }

Ditto

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1329
> +    if (isKnownNotCell(nodeIndex)) {
> +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> +        return allocate();
> +    }

Ditto

[Yuqiang Xian]

(In reply to comment #4)
> (From update of attachment 145923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145923&action=review
> 
> I'm fine with this on principle, but I'm not sure about the calls to terminateSpeculativeExecution().  These days DFG::AbstractState must know exactly where OSR exits may happen since this information is consumed by store elimination.  Hence, any calls to speculationCheck() and terminateSpeculativeExecution() ought to be guarded by checks against things that AbstractState would know during the CFA phase, to ensure that we don't end up performing store elimination under the assumption that there is no OSR exit only to have an OSR exit inserted by the backend.
> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1043
> > +    if (isKnownNotInteger(nodeIndex)) {
> > +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> > +        returnFormat = DataFormatInteger;
> > +        return allocate();
> > +    }
> > +
> 
> Perhaps it's better to use isInt32Prediction(m_state.forNode(nodeIndex).m_type) here?

Ok. I will double check this considering the recent DFG changes. Thanks.

[Yuqiang Xian]

(In reply to comment #4)
> (From update of attachment 145923 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145923&action=review
> 
> I'm fine with this on principle, but I'm not sure about the calls to terminateSpeculativeExecution().  These days DFG::AbstractState must know exactly where OSR exits may happen since this information is consumed by store elimination.  Hence, any calls to speculationCheck() and terminateSpeculativeExecution() ought to be guarded by checks against things that AbstractState would know during the CFA phase, to ensure that we don't end up performing store elimination under the assumption that there is no OSR exit only to have an OSR exit inserted by the backend.

I see assertions are planted in terminateSpeculativeExecution()?

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1043
> > +    if (isKnownNotInteger(nodeIndex)) {
> > +        terminateSpeculativeExecution(Uncountable, JSValueRegs(), NoNode);
> > +        returnFormat = DataFormatInteger;
> > +        return allocate();
> > +    }
> > +
> 
> Perhaps it's better to use isInt32Prediction(m_state.forNode(nodeIndex).m_type) here?

This might not be true. In this case we want to terminate the speculative execution if we know that the child is definitely *not* an integer (or boolean, cell). Examining the generation info directly may be more straightforward and safe, e.g., if we know the info format is (JS)Cell/(JS)Boolean or (JS)Double, we don't need any speculation check here but just forcing terminate the execution. if we rely on the predictions we may still need to plant a speculation check here.

[Anders Carlsson]

Comment on attachment 145924 [details]
o... the previous one is wrongly rebased.. this one please

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.