[webkit-reviews] review denied: [Bug 79899] DFG BasicBlocks should not require that their nodes have contiguous indices in the graph : [Attachment 129614] Patch for review

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Yuqiang Xian
<yuqiang.xian at intel.com>'s request for review:
Bug 79899: DFG BasicBlocks should not require that their nodes have contiguous
indices in the graph
https://bugs.webkit.org/show_bug.cgi?id=79899

Attachment 129614: Patch for review
https://bugs.webkit.org/attachment.cgi?id=129614&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=129614&action=review


This is really awesome.  R- but only because I want to make sure we do CSE
right.

> Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:720
>      
>      void performBlockCSE(BasicBlock& block)
>      {
> -	   m_start = block.begin;
> -	   NodeIndex end = block.end;
> -	   for (m_compileIndex = m_start; m_compileIndex < end;
++m_compileIndex)
> +	   m_start = block[block.startExcludingPhis];
> +	   for (unsigned i = block.startExcludingPhis; i < block.size(); ++i) {

> +	       m_compileIndex = block[i];
>	       performNodeCSE(m_graph[m_compileIndex]);
> +	   }
>      }
>      
>      NodeIndex m_start;

I'm somewhat surprised that these are the only changes in CSE!	CSE has this
whole performance hack where it uses the NodeIndex of the instruction that is a
candidate for elimination, and the NodeIndices of its children, to determine
the range that it will search.

Seems like that relies on NodeIndices of things in a basic block being
monotonically increasing.

Would be better to instead make all of the search things instead break early
when they encounter a NodeIndex at block[i] that matches the NodeIndex in one
of the children of the node from which the search originated.