[webkit-reviews] review denied: [Bug 180852] The CleanUp after LICM is erroneously removing a Check : [Attachment 329435] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Saam Barati <sbarati at apple.com>'s
request for review:
Bug 180852: The CleanUp after LICM is erroneously removing a Check
https://bugs.webkit.org/show_bug.cgi?id=180852

Attachment 329435: patch

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




--- Comment #12 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 329435
  --> https://bugs.webkit.org/attachment.cgi?id=329435
patch

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

I think this could be simpler.

> Source/JavaScriptCore/dfg/DFGAtTailAbstractState.h:67
> +    void setProofStatus(Edge&, ProofStatus) { }

What if this set it to NeedsCheck?  Then you don’t need more data structures.

> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:313
> +	   m_hoistedNodes.add(node);

I don’t think you need this.

> Source/JavaScriptCore/dfg/DFGLICMPhase.cpp:354
> +    HashSet<Node*> m_hoistedNodes;

And if you did need it, it should probably be a vector. Optimizing away
duplicates isn’t going to help you.