Bug 152669 – B3 patchpoints should allow requesting scratch registers

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

[Filip Pizlo]

You should be able to say "I want N scratch registers" and your wish should be granted.

[Filip Pizlo]

Created attachment 268150 [details]
it's taking shape!

[Filip Pizlo]

Created attachment 268154 [details]
more

[Filip Pizlo]

Created attachment 268171 [details]
even more

[Filip Pizlo]

Created attachment 268173 [details]
the patch

[Filip Pizlo]

Comment on attachment 268173 [details]
the patch

This looks like it doesn't break anything.

[WebKit Commit Bot]

Attachment 268173 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:103:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:83:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6171:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6174:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6193:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6207:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6210:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6223:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:56:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:785:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:794:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:850:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 18 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 268174 [details]
the patch

[WebKit Commit Bot]

Attachment 268174 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:103:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:83:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6171:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6174:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6193:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6207:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6210:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6223:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:56:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:785:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:794:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:850:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 18 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 268175 [details]
the patch

More fixes.

[WebKit Commit Bot]

Attachment 268175 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:103:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInstInlines.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:83:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6171:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6174:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6193:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6207:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6210:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:6223:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:228:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:56:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:71:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:146:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:152:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:785:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:794:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:850:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 18 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 268175 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=268175&action=review

I am surprised you don't make the Scratch unspillable in the allocator.

When a scratch Tmp is spilled, it is replaced by a new unspillable Tmp. The next allocation will then selector a better victim for spilling.
I think it would be better to initialize the unspillableTmp from IteratedRegisterCoalescing to avoid building the interference graph twice.

> Source/JavaScriptCore/b3/B3CheckSpecial.cpp:128
> +        && inst.args.size() - m_numCheckArgs - 1 == inst.origin->numChildren() - numB3Args(inst);

Is there a case where both side are not zero?

> Source/JavaScriptCore/b3/B3PatchpointValue.h:63
> +    uint8_t numGPScratchRegisters;
> +    uint8_t numFPScratchRegisters;

uint8_t numGPScratchRegisters { 0 }
instead of setting it up in the constructor? If you change this, you can remove the comment "These are 0 by default." too.

> Source/JavaScriptCore/b3/air/AirInstInlines.h:118
> +template<typename Thing, typename Functor>

I don't think that template works.

If used with StackSlot, this code will try to report extra clobbered regs as StackSlots.

> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:777
> +            build(nullptr, &block->at(0), localCalc);

A bit odd but that makes sense.

> Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:790
>                  // We should not use forEachDefAndExtraClobberedTmp() here since colored Tmps

There is no forEachDefAndExtraClobberedTmp() anymore.
Can you please update that comment to remove forEachDefAndExtraClobberedTmp() and say something about not reporting extra clobbered tmps.

> Source/JavaScriptCore/b3/air/AirLiveness.h:234
>              // First handle def's.

No longer "First" :)

[Filip Pizlo]

(In reply to comment #11)
> Comment on attachment 268175 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268175&action=review
> 
> I am surprised you don't make the Scratch unspillable in the allocator.

I don't think that's straightforward.  Thanks for bring this up, though - thinking about this revealed a bug.

> 
> When a scratch Tmp is spilled, it is replaced by a new unspillable Tmp. 

Exactly.

> The
> next allocation will then selector a better victim for spilling.
> I think it would be better to initialize the unspillableTmp from
> IteratedRegisterCoalescing to avoid building the interference graph twice.

There is no constraint that the Tmp that you use for the scratch argument is used *only* for that scratch argument.  It may be also used elsewhere.  That use may be meaningful.  For example:

Foo Scratch:%tmp42 // first write to %tmp42 and then read from it.
Bar Use:%tmp42 // now read from %tmp42 again.

Currently the code has a bug when this happens and %tmp42 is spilled: we don't emit a store to %tmp42's spill slot after Foo.  I'll fix that.  Note that if we emit such a store and %tmp42 is actually dead after Foo (i.e. there is no Bar) then we'll need some optimization to remove the store later.  I'm OK with this, since anyway this is an existing bug for any dead def that gets spilled.  We just need something akin to eliminateDeadCode() to remove such dead stores.

On the other hand, I get what you're going for: one possible interpretation of the "Scratch" role is that the value stored into the temporary is not used after.  But I like how currently, the Scratch role is defined in terms of uses and defs.  That has the advantage that phases don't have to special-case Scratch if they don't want to.

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:128
> > +        && inst.args.size() - m_numCheckArgs - 1 == inst.origin->numChildren() - numB3Args(inst);
> 
> Is there a case where both side are not zero?

They are usually not zero.  "inst.args.size() - m_numCheckArgs - 1" is the number of stackmap arguments (i.e. OSR exit state) to the check.  Except for some tests in testb3, a Check will usually have a non-zero number of stackmap arguments.

> 
> > Source/JavaScriptCore/b3/B3PatchpointValue.h:63
> > +    uint8_t numGPScratchRegisters;
> > +    uint8_t numFPScratchRegisters;
> 
> uint8_t numGPScratchRegisters { 0 }
> instead of setting it up in the constructor? If you change this, you can
> remove the comment "These are 0 by default." too.

Yeah, I'll do that.

> 
> > Source/JavaScriptCore/b3/air/AirInstInlines.h:118
> > +template<typename Thing, typename Functor>
> 
> I don't think that template works.

It works for Arg and Tmp, and we use it for both of those things. :-)

> 
> If used with StackSlot, this code will try to report extra clobbered regs as
> StackSlots.

I'll add a comment that Thing cannot be StackSlot* for the extra clobbered variant.  That's probably OK - it would be odd for someone to ask for extra clobbered registers when they are asking for stack slots!

> 
> > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:777
> > +            build(nullptr, &block->at(0), localCalc);
> 
> A bit odd but that makes sense.
> 
> > Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp:790
> >                  // We should not use forEachDefAndExtraClobberedTmp() here since colored Tmps
> 
> There is no forEachDefAndExtraClobberedTmp() anymore.
> Can you please update that comment to remove
> forEachDefAndExtraClobberedTmp() and say something about not reporting extra
> clobbered tmps.

Done.

> 
> > Source/JavaScriptCore/b3/air/AirLiveness.h:234
> >              // First handle def's.
> 
> No longer "First" :)

Fixed.

[Filip Pizlo]

> > The
> > next allocation will then selector a better victim for spilling.
> > I think it would be better to initialize the unspillableTmp from
> > IteratedRegisterCoalescing to avoid building the interference graph twice.
> 
> There is no constraint that the Tmp that you use for the scratch argument is
> used *only* for that scratch argument.  It may be also used elsewhere.  That
> use may be meaningful.  For example:
> 
> Foo Scratch:%tmp42 // first write to %tmp42 and then read from it.
> Bar Use:%tmp42 // now read from %tmp42 again.
> 
> Currently the code has a bug when this happens and %tmp42 is spilled: we
> don't emit a store to %tmp42's spill slot after Foo.  I'll fix that.  Note
> that if we emit such a store and %tmp42 is actually dead after Foo (i.e.
> there is no Bar) then we'll need some optimization to remove the store
> later.  I'm OK with this, since anyway this is an existing bug for any dead
> def that gets spilled.  We just need something akin to eliminateDeadCode()
> to remove such dead stores.
> 
> On the other hand, I get what you're going for: one possible interpretation
> of the "Scratch" role is that the value stored into the temporary is not
> used after.  But I like how currently, the Scratch role is defined in terms
> of uses and defs.  That has the advantage that phases don't have to
> special-case Scratch if they don't want to.

I'll file a bug about having IRC mark as unspillable any Tmp that is used only from a !admitsStack Scratch role.

Note that this will probably not matter at all for the foreseeable future.  Scratch registers are super necessary for binary snippets to work, and we need that to run JS code, but the code we generate for the snippets doesn't have to be great and we won't be under any compile time pressure when we do this.

https://bugs.webkit.org/show_bug.cgi?id=152699

[Filip Pizlo]

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

Build fix landed in http://trac.webkit.org/changeset/194543.