Actually we almost support that except for some minor things. With those fixed the virtual register spilling could be cleaner.
Created attachment 137872 [details] patch Also, take the chance to make the filling cleaner especially for the 64bit.
Created attachment 137873 [details] Performance result This is performance neutral.
Created attachment 145923 [details] Rebase
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?
Created attachment 145924 [details] o... the previous one is wrongly rebased.. this one please
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
(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.
(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.
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.