[webkit-reviews] review denied: [Bug 192908] StrengthReductionPhase ValueAdd rule handles BigInt handleCommutativity is breaking JSC::DFG::Edge::useKind() : [Attachment 357768] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <yusukesuzuki at slowstart.org> has denied Caio Lima
<ticaiolima at gmail.com>'s request for review:
Bug 192908: StrengthReductionPhase ValueAdd rule handles BigInt
handleCommutativity is breaking JSC::DFG::Edge::useKind()
https://bugs.webkit.org/show_bug.cgi?id=192908

Attachment 357768: Patch

https://bugs.webkit.org/attachment.cgi?id=357768&action=review




--- Comment #4 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 357768
  --> https://bugs.webkit.org/attachment.cgi?id=357768
Patch

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

>>> Source/JavaScriptCore/ChangeLog:10
>>> +	     existence of both nodes before using m_node->binaryUseKind.
>> 
>> I don't understand: when would ValueAdd ever not have children nodes?  Also,
please provide a test case.  The test will help illustrate the issue.
> 
> Oh, I see there are already failing tests.  I'm still surprised that ValueAdd
has edges that don't have nodes.  Perhaps there's something I don't know.

Yeah, I don't think the current situation is right. ValueAdd must have two
children.
If it happens, it's a bug. We should fix that bug instead of checking
`child1().node() && child2().node()`.