This is an archive of the discontinued LLVM Phabricator instance.

Factor out MachineBlockPlacement::fillWorkLists. NFC
ClosedPublic

Authored by deadalnix on Mar 11 2016, 12:47 AM.

Details

Summary

There are places in MachineBlockPlacement where a worklist is filled in pretty much identical way. The code is duplicated. This refactor it so that the same code is used in both scenarii.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 50395.Mar 11 2016, 12:47 AM
deadalnix retitled this revision from to Factor out MachineBlockPlacement::fillWorkLists. NFC.
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
mcrosier added inline comments.
lib/CodeGen/MachineBlockPlacement.cpp
591

Should this be a continue?

593

Same as above. continue?

598

If you don't mind, I'd prefer to stick with the style of the original code.

if (Chain.UnscheduledPredecessors == 0)
  BlockWorkList.push_back(*Chain.begin());
deadalnix added inline comments.Mar 11 2016, 11:46 AM
lib/CodeGen/MachineBlockPlacement.cpp
593

You are right. How come this didn't broke any tests ? It seems like a pretty bad breakage.

598

I changed it because of D17625 (which this diff is extracted from).

deadalnix updated this revision to Diff 50462.Mar 11 2016, 12:13 PM

Use continue where apropriate.

mcrosier added inline comments.Mar 11 2016, 12:23 PM
lib/CodeGen/MachineBlockPlacement.cpp
593

Perhaps it would be a good idea to add such a test case.

598

Was this style suggested in D17625?

deadalnix added inline comments.Mar 11 2016, 12:36 PM
lib/CodeGen/MachineBlockPlacement.cpp
598

D17625 introduce 2 worklist, so you need to add code there to choose which worklist you add the MBB to.

mcrosier added inline comments.Mar 11 2016, 1:18 PM
lib/CodeGen/MachineBlockPlacement.cpp
598

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?

deadalnix added inline comments.Mar 11 2016, 1:38 PM
lib/CodeGen/MachineBlockPlacement.cpp
593

Do you have a suggestion on how to build such test case ?

598

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.

davidxl added inline comments.
lib/CodeGen/MachineBlockPlacement.cpp
598

I agree with Chad here -- unless it helps reducing if nesting level, early return here does not help readability -- the original code seems more readable:

if (Chain.UnscheduledPredecessors == 0) {

// update worklist(s)

}
return;

deadalnix updated this revision to Diff 50515.Mar 11 2016, 9:31 PM

Style line 598

davidxl accepted this revision.Mar 14 2016, 9:26 AM
davidxl added a reviewer: davidxl.
davidxl added inline comments.
lib/CodeGen/MachineBlockPlacement.cpp
277

Though not done for existing methods, I think it is better to add a slightly detailed comments for this method. Otherwise the refactoring looks good.

This revision is now accepted and ready to land.Mar 14 2016, 9:26 AM
This revision was automatically updated to reflect the committed changes.