Say a patchpoint clobbers every register but regT0 (for the sake of argument), and it also says it clobbers a pinned register. Note that the registerCount() won't include the pinned register. However, the Tmp that the patch defines will have a degree count of registerCount(), instead of registerCount() - 1. This is because we model clobbering of pinned registers in the normal interference graph. Instead, we should not do this. We should model it like we model interference with the FP register. The interference graph should only model interference edges between Tmps and Tmps, and Tmps and assignable registers.
Created attachment 309676 [details] WIP I still have some concerns on us deciding to coalesce pinned regs. E.g: ``` Move pinnedGPR, temp1 Patch # writePinned Move pinnedGPR, temp2 use temp1 use temp2 ``` Can we transform this into: ``` Patch use pinnedGPR use pinnedGPR ```
Comment on attachment 309676 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=309676&action=review I think this looks OK overall. It would be good to use something other than a vector for tracking the set of pinned regs, but then again, it probably doesn't matter very much. You should change the names of those functiuons in Code/Procedure, though - the correct term is "mutable GPRs/FPRs". > Source/JavaScriptCore/b3/B3Procedure.h:262 > + // This is just used for tests, and is not meant to be efficient. > + // We could make it more efficient later if we wanted to. > + JS_EXPORT_PRIVATE RegisterSet gprBank(); > + JS_EXPORT_PRIVATE RegisterSet fprBank(); "Bank" is the name of an enumeration that has GP and FP as its members. That enum is relatively new, but that's roughly how we've always thought of the word. So, I think you should pick a different name for these functions. > Source/JavaScriptCore/b3/air/AirAllocateRegistersByGraphColoring.cpp:115 > + for (const IndexType pinnedReg : m_pinnedRegs) { > + if (pinnedReg == tmpIndex) > + return true; > + } Why not make this a bitvector lookup? > Source/JavaScriptCore/b3/air/AirCode.cpp:111 > +RegisterSet Code::gprBank() > +{ > + RegisterSet result; > + for (Reg reg : m_gpRegsInPriorityOrder) > + result.set(reg); > + return result; > +} > + > +RegisterSet Code::fprBank() > +{ > + RegisterSet result; > + for (Reg reg : m_fpRegsInPriorityOrder) > + result.set(reg); > + return result; > } Why aren't you just using Code::mutableRegs()? If you want the GP subset, you filter it with RegisterSet::allGPRs(). If you want the FP subset, you filter it with RegisterSet::allFPRs(). I think that means that if you want to provide helpers to do this for you, they should be called mutableGPRs() and mutableFPRs(), not gprBank/fprBank.
(In reply to Saam Barati from comment #1) > Created attachment 309676 [details] > WIP > > I still have some concerns on us deciding to coalesce pinned regs. E.g: > > ``` > Move pinnedGPR, temp1 > Patch # writePinned > Move pinnedGPR, temp2 > use temp1 > use temp2 > ``` > > Can we transform this into: > ``` > Patch > use pinnedGPR > use pinnedGPR > ``` If Patch writesPinned then (unless the user is doing shenanigans) the Patch will also clobber the pinnedGPR. Therefore, temp1 will not get coalesced with pinnedGPR, but temp2 could be.
(In reply to Saam Barati from comment #1) > Created attachment 309676 [details] > WIP > > I still have some concerns on us deciding to coalesce pinned regs. E.g: > > ``` > Move pinnedGPR, temp1 > Patch # writePinned > Move pinnedGPR, temp2 > use temp1 > use temp2 > ``` Actually, my analysis here is wrong. IRC can not coalesce these regardless. Let's forget about pinned registers here, and just consider this program: ``` Move %tmp0, %tmp1 Move %tmp0, %tmp2 Add32 %tmp1, %tmp2, %r0 Ret32 %r0 ``` Interestingly, IRC can not coalesce both moves. To understand why, note that tmp1 is live in to the second move. This means that tmp2 and tmp1 can't be assigned to the same register given how we build the interference graph. Clearly this shows a weakness in IRC. For example, it'll make this transformation: ``` Move %tmp0, %tmp1 Add32 %tmp1, %tmp0, %r0 Ret32 %r0 ``` But obviously this can even be further reduced to be this if we ran IRC over again: ``` Add32 %tmp0, %tmp0, %r0 Ret32 %r0 ``` > > Can we transform this into: > ``` > Patch > use pinnedGPR > use pinnedGPR > ```
Created attachment 309994 [details] WIP This patch generalizes the m_interferesWithFramePointer concept to include all pinned registers. That said, I'm starting to thing that this approach is hacky. I think this whole notion could be modeled much more naturally: We start by keeping pinned registers accounted for in registerCount(). Then, we just treat all pinned registers as always being alive. Then, we just model this as IRC normally would. I think this will just work for coalescing, etc, and be more consistent w/ the framework of IRC. I need to think about it a bit more to make sure I'm right. I'm gonna test it out now.
Created attachment 309995 [details] WIP for alternate modeling The only difference worth reading is in AirAllocateRegistersByGraphColoring
Created attachment 310106 [details] patch
Attachment 310106 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/testair.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 310107 [details] patch
Attachment 310107 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/testair.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 310107 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310107&action=review > Source/JavaScriptCore/b3/B3Procedure.h:260 > + // This is just used for tests, and is not meant to be efficient. > + // We could make it more efficient later if we wanted to. Drive by comment: I suggest making this comment plural since you're referring to both mutableGPRs and mutableFPRs: // These are just used for tests, and are not meant to be efficient. // We could make them more efficient later if we wanted to. Using singular form implies the comment only applies to the first one. > Source/JavaScriptCore/b3/air/AirCode.h:313 > + // This is just used for tests, and is not meant to be efficient. > + // We could make it more efficient later if we wanted to. Ditto.
Comment on attachment 310107 [details] patch This is rad. I like how simple this turned out.
Comment on attachment 310107 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=310107&action=review >> Source/JavaScriptCore/b3/B3Procedure.h:260 >> + // We could make it more efficient later if we wanted to. > > Drive by comment: I suggest making this comment plural since you're referring to both mutableGPRs and mutableFPRs: > > // These are just used for tests, and are not meant to be efficient. > // We could make them more efficient later if we wanted to. > > Using singular form implies the comment only applies to the first one. Ima just remove this comment, it preexists the code as I wrote it. I had it because a previous version of this function was worse. This function does pretty standard RegisterSet manipulations.
Created attachment 310334 [details] patch for landing
Attachment 310334 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/testair.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 310334 [details] patch for landing Attachment 310334 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3758268 New failing tests: http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html
Created attachment 310355 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 310334 [details] patch for landing Failures look unrelated.
Comment on attachment 310334 [details] patch for landing Clearing flags on attachment: 310334 Committed r216989: <http://trac.webkit.org/changeset/216989>
All reviewed patches have been landed. Closing bug.