Bug 151809 – B3 patchpoints should allow specifying output constraints

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 266560 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/testb3.cpp:3734:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3736:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3745:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3760:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3762:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3771:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3788:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3790:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:3801:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
Total errors found: 9 in 13 files


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

[Benjamin Poulain]

Comment on attachment 266560 [details]
the patch

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

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1711
> +                    after.append(Inst(
> +                        relaxedMoveForType(patchpointValue->type()), m_value, reg, tmp(patchpointValue)));

Either move it to one line or give a temporary name to that Inst.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1718
> +                    after.append(Inst(
> +                        moveForType(patchpointValue->type()), m_value, arg, tmp(patchpointValue)));

Ditto

> Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:197
> +        // We already verified this above.

Stale comment now.

> Source/JavaScriptCore/b3/B3StackmapValue.h:102
> +    // assume to know anything about what kind of value might have been stored. In B3's model of

Maybe "Air's model"?
The B3 IR does not care about registers.

> Source/JavaScriptCore/b3/B3StackmapValue.h:126
> +    // Note that a common use case of early clobber sets is to indicate that this is the set of registers
> +    // that shall not be used for inputs to the value. But B3 supports two different ways of specifying
> +    // this, the other being LateUse (not yet available to stackmaps directly, FIXME:

This does not sound right.

LateUse prevent us to use the register for anything clobbered and the results, but it can be an input to the value.

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 266560 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266560&action=review
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:1711
> > +                    after.append(Inst(
> > +                        relaxedMoveForType(patchpointValue->type()), m_value, reg, tmp(patchpointValue)));
> 
> Either move it to one line or give a temporary name to that Inst.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:1718
> > +                    after.append(Inst(
> > +                        moveForType(patchpointValue->type()), m_value, arg, tmp(patchpointValue)));
> 
> Ditto
> 
> > Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:197
> > +        // We already verified this above.
> 
> Stale comment now.

Not quite.  "Above" is the other method, isArgValidForValue().  You're expected to also call that one.  I'll change the comment to make it clear that it's referring to the other method.

> 
> > Source/JavaScriptCore/b3/B3StackmapValue.h:102
> > +    // assume to know anything about what kind of value might have been stored. In B3's model of
> 
> Maybe "Air's model"?
> The B3 IR does not care about registers.

Well, B3 IR does care about registers in the sense that StackmapValue allows you to specify the registers used for the inputs and output.  So, I think it's meaningful to talk about B3 having a model about what happens to registers before and after a StackmapValue.

> 
> > Source/JavaScriptCore/b3/B3StackmapValue.h:126
> > +    // Note that a common use case of early clobber sets is to indicate that this is the set of registers
> > +    // that shall not be used for inputs to the value. But B3 supports two different ways of specifying
> > +    // this, the other being LateUse (not yet available to stackmaps directly, FIXME:
> 
> This does not sound right.
> 
> LateUse prevent us to use the register for anything clobbered and the
> results, but it can be an input to the value.

I fixed the comment to make it clear that it refers to LateUse in combination with late clobber.

[Filip Pizlo]

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