Bug 162692 - B3 opcodes should leave room for flags
Summary: B3 opcodes should leave room for flags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 162689
  Show dependency treegraph
 
Reported: 2016-09-28 10:54 PDT by Filip Pizlo
Modified: 2016-09-29 11:47 PDT (History)
5 users (show)

See Also:


Attachments
turning chill into a kind bit (29.06 KB, patch)
2016-09-28 11:55 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (64.40 KB, patch)
2016-09-28 14:41 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (81.48 KB, patch)
2016-09-28 19:22 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (99.46 KB, patch)
2016-09-28 21:36 PDT, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-09-28 10:54:08 PDT
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.
Comment 1 Filip Pizlo 2016-09-28 11:55:09 PDT
Created attachment 290105 [details]
turning chill into a kind bit

This is fun!
Comment 2 Filip Pizlo 2016-09-28 13:24:33 PDT
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.
Comment 3 Filip Pizlo 2016-09-28 14:41:58 PDT
Created attachment 290123 [details]
more
Comment 4 Filip Pizlo 2016-09-28 19:22:43 PDT
Created attachment 290162 [details]
the patch
Comment 5 WebKit Commit Bot 2016-09-28 19:23:57 PDT
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.
Comment 6 Filip Pizlo 2016-09-28 21:36:18 PDT
Created attachment 290176 [details]
the patch
Comment 7 WebKit Commit Bot 2016-09-28 21:37:55 PDT
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.
Comment 8 Keith Miller 2016-09-28 22:09:22 PDT
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?
Comment 9 Filip Pizlo 2016-09-28 22:22:22 PDT
(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.
Comment 10 Keith Miller 2016-09-29 10:48:38 PDT
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?
Comment 11 Filip Pizlo 2016-09-29 10:52:12 PDT
(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.
Comment 12 Filip Pizlo 2016-09-29 10:55:32 PDT
(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.
Comment 13 Filip Pizlo 2016-09-29 11:47:50 PDT
Landed in https://trac.webkit.org/changeset/206595