Bug 162692 – B3 opcodes should leave room for flags

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

[Filip Pizlo]

We want to be able to express variants of opcodes using some meta-data in the opcode itself.  This is great in B3 because then this meta-data could become part of the accepts() logic for dynamic casting Values.  This is great in Air because then we don't have to add more fields to Inst - the opcode field would simply expand to tell us this extra information, leading to hopefully a cleaner overall design.  For example a "trapping" flag is not meaningful for every opcode, so we don't want a trapping bit in Inst; we want a smart Opcode-like class that will reveal a trapping flag exactly when the opcode makes it meaningful, and intelligently reuses those bits for other things for opcodes that don't care about trapping but maybe care about other things.

[Filip Pizlo]

Created attachment 290105 [details]
turning chill into a kind bit

This is fun!

[Filip Pizlo]

I'm not convinced that Air should use the same Kind-based thing as B3.  Maybe Air should just have an extra flags field in Inst.

[Filip Pizlo]

Created attachment 290123 [details]
more

[Filip Pizlo]

Created attachment 290162 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/B3Value.h:424:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:430:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:436:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:442:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:448:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:63:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 36 files


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

[Filip Pizlo]

Created attachment 290176 [details]
the patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/b3/B3Value.h:424:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:430:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:436:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:442:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:448:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3ValueKey.h:63:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 38 files


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

[Keith Miller]

Comment on attachment 290176 [details]
the patch

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

r- for now until my question is answered.

> Source/JavaScriptCore/ChangeLog:30
> +        are only interested in the kind of arithmetic being done and not the chillness.

Is chillness the technical term? :)

> Source/JavaScriptCore/ChangeLog:53
> +        This change addresses this problem by making the compiler pass around Kinds rather than
> +        Opcodes. A Kind contains an Opcode as well as any number of opcode-specific bits. This
> +        change demonstrates how Kind should be used by converting chillness to it. Kind has
> +        hasIsChill(), isChill(), and setIsChill() methods. hasIsChill() is true only for Div and
> +        Mod. setIsChill() asserts if !hasIsChill(). If you want to create a Chill Div, you say
> +        chill(Div). IR dumps will print it like this:

I feel like one downside to this API is that it can be hard to figure out which bits are supported by which opcodes. This is one of the most annoying things about the DFG, where one has no idea how many Edges or OpInfos a node has and even if you do its super hard to figure out what those Edges/OpInfos do. While I like that they are named here rather than the DFG system of trying to guess what child3() is, I'm wondering if it might be better if there was a struct for each opcode that told you what the bits for that opcode are. Essentially, making Kinds more like an ADT. What do you think the pros and cons are?

[Filip Pizlo]

(In reply to comment #8)
> Comment on attachment 290176 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290176&action=review
> 
> r- for now until my question is answered.
> 
> > Source/JavaScriptCore/ChangeLog:30
> > +        are only interested in the kind of arithmetic being done and not the chillness.
> 
> Is chillness the technical term? :)

It's become one.  It struck enough of a chord that it's referred to in wasm meetings.

> 
> > Source/JavaScriptCore/ChangeLog:53
> > +        This change addresses this problem by making the compiler pass around Kinds rather than
> > +        Opcodes. A Kind contains an Opcode as well as any number of opcode-specific bits. This
> > +        change demonstrates how Kind should be used by converting chillness to it. Kind has
> > +        hasIsChill(), isChill(), and setIsChill() methods. hasIsChill() is true only for Div and
> > +        Mod. setIsChill() asserts if !hasIsChill(). If you want to create a Chill Div, you say
> > +        chill(Div). IR dumps will print it like this:
> 
> I feel like one downside to this API is that it can be hard to figure out
> which bits are supported by which opcodes. 

The bit's hasBlah() method will tell you.

> This is one of the most annoying
> things about the DFG, where one has no idea how many Edges or OpInfos a node
> has and even if you do its super hard to figure out what those Edges/OpInfos
> do. While I like that they are named here rather than the DFG system of
> trying to guess what child3() is, I'm wondering if it might be better if
> there was a struct for each opcode that told you what the bits for that
> opcode are. Essentially, making Kinds more like an ADT. What do you think
> the pros and cons are?

Can you be a lot more detailed about what you're proposing?

This is radically different than DFG children, which are numbered and not checked at all.  It's also not like DFG's OpInfo's, which are not checked during Node creation (though like Kind's members, they are checked on access). I'm a strong proponent of Kind-style dynamic yet fixed-size objects, and they are widespread in B3 already.  ValueKey, ValueRep, and Air::Arg are three examples that immediately come to mind.  There are many more such things throughout JSC.  So, if that is a style you object to, then I think it's not fair to apply this objection just to this patch.  This patch uses a common JSC idiom.

To be clear, Kind has to be one fixed-size type.  We can't have subclasses of Kind.  B3::Value has a m_kind field and it uses this field as the type ID for determining which Value subclass you are.  That's how B3's dynamic casts work.  So, it's very important that Kind itself doesn't require a type hierarchy, since that would require a level of meta-circularity that I don't want to have to think about.

[Keith Miller]

Comment on attachment 290176 [details]
the patch

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

After online discussion I'm withdrawing my previous objection since I think it's not mutually exclusive to the current API. 

So, r=me with a comment.

> Source/JavaScriptCore/b3/B3Kind.h:150
> +    bool operator<(const Kind& other) const
> +    {
> +        if (m_opcode == other.m_opcode)
> +            return u.bits < other.u.bits;
> +        return m_opcode < other.m_opcode;
> +    }
> +    
> +    bool operator>(const Kind& other) const
> +    {
> +        return other < *this;
> +    }
> +    
> +    bool operator<=(const Kind& other) const
> +    {
> +        return !(*this > other);
> +    }
> +    
> +    bool operator>=(const Kind& other) const
> +    {
> +        return !(*this < other);
> +    }

What are these used for. If they aren't used maybe we should delete them?

[Filip Pizlo]

(In reply to comment #10)
> Comment on attachment 290176 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290176&action=review
> 
> After online discussion I'm withdrawing my previous objection since I think
> it's not mutually exclusive to the current API. 
> 
> So, r=me with a comment.
> 
> > Source/JavaScriptCore/b3/B3Kind.h:150
> > +    bool operator<(const Kind& other) const
> > +    {
> > +        if (m_opcode == other.m_opcode)
> > +            return u.bits < other.u.bits;
> > +        return m_opcode < other.m_opcode;
> > +    }
> > +    
> > +    bool operator>(const Kind& other) const
> > +    {
> > +        return other < *this;
> > +    }
> > +    
> > +    bool operator<=(const Kind& other) const
> > +    {
> > +        return !(*this > other);
> > +    }
> > +    
> > +    bool operator>=(const Kind& other) const
> > +    {
> > +        return !(*this < other);
> > +    }
> 
> What are these used for. If they aren't used maybe we should delete them?

I want it to be easy to sort on kind, just like you can sort on opcode.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 290176 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=290176&action=review
> > 
> > After online discussion I'm withdrawing my previous objection since I think
> > it's not mutually exclusive to the current API. 
> > 
> > So, r=me with a comment.
> > 
> > > Source/JavaScriptCore/b3/B3Kind.h:150
> > > +    bool operator<(const Kind& other) const
> > > +    {
> > > +        if (m_opcode == other.m_opcode)
> > > +            return u.bits < other.u.bits;
> > > +        return m_opcode < other.m_opcode;
> > > +    }
> > > +    
> > > +    bool operator>(const Kind& other) const
> > > +    {
> > > +        return other < *this;
> > > +    }
> > > +    
> > > +    bool operator<=(const Kind& other) const
> > > +    {
> > > +        return !(*this > other);
> > > +    }
> > > +    
> > > +    bool operator>=(const Kind& other) const
> > > +    {
> > > +        return !(*this < other);
> > > +    }
> > 
> > What are these used for. If they aren't used maybe we should delete them?
> 
> I want it to be easy to sort on kind, just like you can sort on opcode.

Look like nobody does that.  I'll remove.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/206595