Created attachment 259362 [details] Patch

Comment on attachment 259362 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259362&action=review > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:265 > + unsigned registerCount = registerSaveLocations->size(); > + for (unsigned i = 0; i < registerCount; i++) { > + RegisterAtOffset currentEntry = registerSaveLocations->at(i); Nit: we could (define and) use an iterator wrapping over the underlying WTF::Vector::iterator here and in other places. > Source/JavaScriptCore/ftl/FTLUnwindInfo.h:38 > +class UnwindInfo : public RegisterAtOffsetList { It looks like this class doesn't really need to be a class anymore. Why not have a RegisterAtOffsetList parseUnwindInfo(void*, size_t, GeneratedFunction) helper instead (maybe returning a std::pair<bool, RegisterAtOffsetList> or std::unique_ptr<RegisterAtOffsetList> instead)? > Source/JavaScriptCore/jit/RegisterAtOffset.h:53 > + int offsetAsIndex() const { return offset() / sizeof(void*); } You never use this. > Source/JavaScriptCore/jit/RegisterAtOffsetList.cpp:39 > + m_registers = Vector<RegisterAtOffset>(other.m_registers); I believe you can just say m_registers = other.m_registers. > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:41 > + RegisterAtOffsetList(const RegisterAtOffsetList&); Why can't we use the compiler-generated default? > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:43 > + ~RegisterAtOffsetList(); Ditto. > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:57 > + RegisterAtOffset& at(size_t index) I think you should either implement operator[] or call m_registers.at(index) - even though they are effectively the same function - for the sake of consistency. > Source/JavaScriptCore/jit/RegisterSet.cpp:244 > + // LLVM might save and use all ARM64 callee saves specified in the ABI. What about floating point callee-save registers?

Created attachment 259391 [details] Updated patch (In reply to comment #2) > Comment on attachment 259362 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259362&action=review > > > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:265 > > + unsigned registerCount = registerSaveLocations->size(); > > + for (unsigned i = 0; i < registerCount; i++) { > > + RegisterAtOffset currentEntry = registerSaveLocations->at(i); > > Nit: we could (define and) use an iterator wrapping over the underlying > WTF::Vector::iterator here and in other places. I was just following the existing usages. > > Source/JavaScriptCore/ftl/FTLUnwindInfo.h:38 > > +class UnwindInfo : public RegisterAtOffsetList { > > It looks like this class doesn't really need to be a class anymore. Why not > have a RegisterAtOffsetList parseUnwindInfo(void*, size_t, > GeneratedFunction) helper instead (maybe returning a std::pair<bool, > RegisterAtOffsetList> or std::unique_ptr<RegisterAtOffsetList> instead)? I made it a stand alone function. It sets the CodeBlock's RegisterAtOffsetList directly. Eliminated UnwindInfo and changed all references to use the callee saves stored in the CodeBlock. > > Source/JavaScriptCore/jit/RegisterAtOffset.h:53 > > + int offsetAsIndex() const { return offset() / sizeof(void*); } > > You never use this. I put it in there, but did the conversion to index explicitly elsewhere. I'm now using it. > > Source/JavaScriptCore/jit/RegisterAtOffsetList.cpp:39 > > + m_registers = Vector<RegisterAtOffset>(other.m_registers); > > I believe you can just say m_registers = other.m_registers. Fixed. > > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:41 > > + RegisterAtOffsetList(const RegisterAtOffsetList&); > > Why can't we use the compiler-generated default? Done. > > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:43 > > + ~RegisterAtOffsetList(); > > Ditto. Removed. > > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:57 > > + RegisterAtOffset& at(size_t index) > > I think you should either implement operator[] or call m_registers.at(index) > - even though they are effectively the same function - for the sake of > consistency. Changed to call m_registers.at(index). > > Source/JavaScriptCore/jit/RegisterSet.cpp:244 > > + // LLVM might save and use all ARM64 callee saves specified in the ABI. > > What about floating point callee-save registers? Register set doesn't have FP callee saves for any platform. Therefore I didn't add the ones for ARM64.

Comment on attachment 259391 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=259391&action=review > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:266 > + if (RegisterSet::stackRegisters().get(currentEntry.reg())) If we cache registerSaveLocations->size(), we should cache RegisterSet::stackRegisters() as well. > Source/JavaScriptCore/ftl/FTLUnwindInfo.cpp:660 > RELEASE_ASSERT(!!section); > if (!section) > return false; This if (!section) will never be reached and should be removed while we're here. I also don't like the name "parseUnwindInfo" if it also has the responsibility to set the CodeBlock's calleeSaveRegisters. What about either: - rename it to parseAndSetUnwindInfo (or something similar) - since it looks like this unreachable if is the only case where we return a non-true value, return a RegisterAtOffsetList (or std::unique_ptr<RegisterAtOffsetList>) instead and call CodeBlock::setCalleeSaveRegisters at the call site > Source/JavaScriptCore/jit/AssemblyHelpers.h:247 > + if (RegisterSet::stackRegisters().get(entry.reg())) If we cache allCalleeSaves->size() we should cache RegisterSet::stackRegisters() as well. > Source/JavaScriptCore/jit/AssemblyHelpers.h:272 > + if (RegisterSet::stackRegisters().get(vmEntry.reg())) Ditto.

Created attachment 259408 [details] Patch with more updates

Committed r188654: <http://trac.webkit.org/changeset/188654>

*** This bug has been marked as a duplicate of bug 148666 ***

Comment on attachment 259408 [details] Patch with more updates Cleared review? from attachment 259408 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).