Bug 171955 – PinnedRegisters should be better modeled in IRC/Briggs

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

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.

[Saam Barati]

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
```

[Filip Pizlo]

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.

[Filip Pizlo]

(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.

[Saam Barati]

(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
> ```

[Saam Barati]

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.

[Saam Barati]

Created attachment 309995 [details]
WIP for alternate modeling

The only difference worth reading is in AirAllocateRegistersByGraphColoring

[Saam Barati]

Created attachment 310106 [details]
patch

[Build Bot]

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.

[Saam Barati]

Created attachment 310107 [details]
patch

[Build Bot]

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.

[Mark Lam]

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.

[Filip Pizlo]

Comment on attachment 310107 [details]
patch

This is rad.  I like how simple this turned out.

[Saam Barati]

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.

[Saam Barati]

Created attachment 310334 [details]
patch for landing

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Saam Barati]

Comment on attachment 310334 [details]
patch for landing

Failures look unrelated.

[WebKit Commit Bot]

Comment on attachment 310334 [details]
patch for landing

Clearing flags on attachment: 310334

Committed r216989: <http://trac.webkit.org/changeset/216989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.