[webkit-reviews] review granted: [Bug 67985] SpeculativeJIT::shouldSpeculateInteger(NodeIndex, NodeIndex) should return false if either node can be double : [Attachment 107217] the patch - fix review, did some refactoring

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 67985: SpeculativeJIT::shouldSpeculateInteger(NodeIndex, NodeIndex) should
return false if either node can be double
https://bugs.webkit.org/show_bug.cgi?id=67985

Attachment 107217: the patch - fix review, did some refactoring
https://bugs.webkit.org/attachment.cgi?id=107217&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107217&action=review


r=me but please don't check in the accidental change to Platform.h.

> Source/JavaScriptCore/ChangeLog:38
> +	   * dfg/DFGGenerationInfo.h:
> +	   (JSC::DFG::isJSFormat):
> +	   (JSC::DFG::isJSInteger):
> +	   (JSC::DFG::isJSDouble):
> +	   (JSC::DFG::isJSCell):
> +	   (JSC::DFG::isJSBoolean):
> +	   (JSC::DFG::GenerationInfo::isJSFormat):
> +	   (JSC::DFG::GenerationInfo::isJSInteger):
> +	   (JSC::DFG::GenerationInfo::isJSDouble):
> +	   (JSC::DFG::GenerationInfo::isJSCell):
> +	   (JSC::DFG::GenerationInfo::isJSBoolean):
> +	   * dfg/DFGJITCodeGenerator.cpp:
> +	   (JSC::DFG::JITCodeGenerator::isKnownInteger):
> +	   (JSC::DFG::JITCodeGenerator::isKnownNumeric):
> +	   (JSC::DFG::JITCodeGenerator::isKnownCell):
> +	   (JSC::DFG::JITCodeGenerator::isKnownNotInteger):
> +	   (JSC::DFG::JITCodeGenerator::isKnownBoolean):
> +	   * dfg/DFGJITCodeGenerator.h:
> +	   * dfg/DFGNonSpeculativeJIT.cpp:
> +	   (JSC::DFG::NonSpeculativeJIT::compile):
> +	   * dfg/DFGSpeculativeJIT.h:
> +	   (JSC::DFG::SpeculativeJIT::isInteger):
> +	   (JSC::DFG::SpeculativeJIT::shouldSpeculateDouble):
> +	   (JSC::DFG::SpeculativeJIT::shouldNotSpeculateInteger):
> +	   (JSC::DFG::SpeculativeJIT::shouldSpeculateInteger):

It's great to put line-by-line comments next to this auto-generated helper
text, but in cases where you don't do that, I think it's better just to delete
the text.

> Source/JavaScriptCore/dfg/DFGGenerationInfo.h:124
> +inline bool isJSFormat(DataFormat format, DataFormat expectedFormat)
> +{
> +    ASSERT(expectedFormat & DataFormatJS);
> +    return (format | DataFormatJS) == expectedFormat;
> +}

This is way better!

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:400
> +	   return !(shouldNotSpeculateInteger(op1) ||
shouldNotSpeculateInteger(op2)) && (shouldSpeculateInteger(op1) ||
shouldSpeculateInteger(op2));

The fight between shouldNotSpeculateInteger() and shouldSpeculateInteger() is a
bit mind-bending, but I don't have a specific suggestion to make right now.

> Source/JavaScriptCore/wtf/Platform.h:968
> -#define ENABLE_TIERED_COMPILATION 0
> +#define ENABLE_TIERED_COMPILATION 1

Hey man, if you want to enable it so badly, why don't you just submit a patch?
;)