[PATCH] D18077: Factor out MachineBlockPlacement::fillWorkLists. NFC

Amaury SECHET via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 11 13:38:47 PST 2016


deadalnix added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:593
@@ +592,3 @@
+      if (BlockToChain[Pred] == &Chain)
+        continue;
+      ++Chain.UnscheduledPredecessors;
----------------
mcrosier wrote:
> Perhaps it would be a good idea to add such a test case.
Do you have a suggestion on how to build such test case ?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:598
@@ +597,3 @@
+
+  if (Chain.UnscheduledPredecessors != 0)
+    return;
----------------
mcrosier wrote:
> deadalnix wrote:
> > mcrosier wrote:
> > > Was this style suggested in D17625?
> > D17625 introduce 2 worklist, so you need to add code there to choose which worklist you add the MBB to.
> Honestly, I don't follow your argument.
> 
> I prefer this style (i.e., the original code):
> 
>   if (Chain.UnscheduledPredecessors == 0)
>     BlockWorkList.push_back(*Chain.begin());
> 
> rather than your change (below):
> 
>   if (Chain.UnscheduledPredecessors != 0)
>     return;
>   BlockWorkList.push_back(*Chain.begin());
> 
> For this patch can we just stick with the original code?
It can. I just would have to change this in D17625 instead of here, which really I don't care. I was asked to extract NFC changes in this patch, so that made sense to include this.


http://reviews.llvm.org/D18077





More information about the llvm-commits mailing list