[webkit-reviews] review granted: [Bug 80415] Eliminate redundant Phis in DFG : [Attachment 130619] updated patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yuqiang Xian
<yuqiang.xian at intel.com>'s request for review:
Bug 80415: Eliminate redundant Phis in DFG
https://bugs.webkit.org/show_bug.cgi?id=80415

Attachment 130619: updated patch
https://bugs.webkit.org/attachment.cgi?id=130619&action=review

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


R=me as it is, but it might be worth considering doing away with the
Phi-to-block links, if possible.

> Source/JavaScriptCore/dfg/DFGRedundantPhiEliminationPhase.cpp:83
> +		   if (replacement != NoNode) {
> +		       // Current Phi node will be skipped, we need to update
> +		       // the variable information if it references this node.
> +		       BasicBlock* basicBlock =
m_graph.m_blocks[node.blockIndex()].get();
> +		       VirtualRegister operand = node.local();
> +		       if (operandIsArgument(operand)) {
> +			   unsigned arg = operandToArgument(operand);
> +			   if (basicBlock->variablesAtHead.argument(arg) ==
index) {
> +			       // This argument must be unused in this block.
> +			       ASSERT(basicBlock->variablesAtTail.argument(arg)
== index);
> +			       basicBlock->variablesAtHead.argument(arg) =
replacement;
> +			       basicBlock->variablesAtTail.argument(arg) =
replacement;
> +			   }
> +		       } else {
> +			   if (basicBlock->variablesAtHead.local(operand) ==
index) {
> +			       // This local variable must be unused in this
block.
> +			      
ASSERT(basicBlock->variablesAtTail.local(operand) == index);
> +			       basicBlock->variablesAtHead.local(operand) =
replacement;
> +			       basicBlock->variablesAtTail.local(operand) =
replacement;
> +			   }
> +		       }

I think this is fine, but why didn't you just walk all blocks'
variablesAtHead|Tail lists after the Phi fixup fixpoint?  That way, you
wouldn't have had to save the block index in the Phi nodes' OpInfos.