Bug 152420

Created attachment 267615 [details] Patch

Attachment 267615 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:870: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 267615 [details] Patch r=me

Comment on attachment 267615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267615&action=review > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:89 > + AbstractIteratedRegisterCoalescingAllocator(const Vector<Reg>& regsInPriorityOrder, IndexType lastPrecoloredRegisterIndex, unsigned tmpArraySize) This should take Code&, and the class should have a field called Code& m_code, and all other helper functions should be members of this class if at all possible. I think that there are two good reasons for this: 1) Consistency. All phases currently make Code& available to all helpers, either by making those helpers be lambdas that close over Code& or by having a helper class that holds a Code& m_code property. Also, all phases currently make it easy to share state with all helper functions, for example if we need to run some analysis and stash the results so that they can be reused throughout the phase. They do it by making all helper functions either be lambdas that close over [&] or by making them member functions inside the helper class. 2) Hackability. It's within bounds to add new code that relies on Code& or on some additional analysis. If we write phases using your style, then anytime we need to modify the phase to make use of some new analysis, we'll have to thread this analysis through all of the functions/classes. If we write phases using the style that all other phases use, then it's easy to make use of new analyses. An example of this is the work I'm doing right now to teach IRC that Move32 is sometimes coalescable. To do that, I need mayBeCoalescable() to have access to Code& plus two other analyses. Had you written IRC using the same style as the other phases, this would have been easy. But now I'm going to essentially have to hack IRC to do what other phases do, or I'll have to thread Code& plus the two other analyses as arguments to all of the standalone functions that call mayBeCoalescable(). That's not fun.

Comment on attachment 267615 [details] Patch Marking r- because in its current form, this refactoring would make it very difficult to add Move32 coalescing.

Benjamin: check out the WIP patch in https://bugs.webkit.org/show_bug.cgi?id=152365. Notice how this adds a TmpWidth member to the main class of IRC, and then it uses it from a lot of places. I want this kind of extension to be easy to add to any phase. Roughly, inside any part of a phase, it should be possible to easily insert code that queries some analysis. It's easy if all of the code in the phase shares some common lexical scope or class instance. It's not easy if the phase has a lot of code in standalone functions. This is why the other phases all try to share some lexical scope or some class instance. In the past, I was ambivalent towards IRC adopting its own style, but now I think that there is an actual argument in favor of using a common style that makes it easy to write these kinds of improvements. Can you do your refactoring in such a way that there is a class that holds all of the code in the phase, and all of the functionality has a pointer to an instance of that class?

Created attachment 267690 [details] Patch

Does that work for you? m_tmpWidth would be ownned by IteratedRegisterCoalescing. It will then be used by each ColoringAllocator to build its own graph.

Attachment 267690 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:773: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:842: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 267690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=267690&action=review > Source/JavaScriptCore/ChangeLog:8 > + AirIteratedRegisterCoalescing has been allocating a bit of mess over time. Typo: allocating->accumulating > Source/JavaScriptCore/ChangeLog:13 > + with half of the function using Tmp, the other using indices. the other -> the other half

Created attachment 267696 [details] Patch for landing

The failing test that is blocking commit-queue is not related to B3. I'm going to land this myself.

Landed in http://trac.webkit.org/changeset/194316