Bug 162692

Created attachment 290105 [details] turning chill into a kind bit This is fun!

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.

Created attachment 290123 [details] more

Created attachment 290162 [details] the patch

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.

Created attachment 290176 [details] the patch

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

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

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

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

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