This is an archive of the discontinued LLVM Phabricator instance.

[MachineScheduler] minor refactoring: new static function checkResourceLimit()
ClosedPublic

Authored by jonpa on Oct 24 2017, 5:10 AM.

Details

Summary

I discovered a while back ago while reading the code in MachineScheduler.cpp that there was some duplicated code that was checking resource limits. I experimented with refactoring it, and this is my result. Does it look like an improvement I should commit?

NFC as far as I can see.

Diff Detail

Event Timeline

jonpa created this revision.Oct 24 2017, 5:10 AM
fhahn added a subscriber: fhahn.Oct 24 2017, 6:54 AM

Looks like a reasonable improvement to me, although I am not entirely sure if adding 2 methods to SchedBoundary is the best thing to do here, as the only thing checkResourceLimit uses from SchedBoundary is SchedModel. Just having static bool checkResourceLimit(unsigned LFactor, unsigned Count, unsigned Lat) seems slightly simpler.

include/llvm/CodeGen/MachineScheduler.h
685 ↗(On Diff #120053)

I think a comment here would be good to avoid confusion with isResourceLimited below. Also, Lat -> Latency ?

687 ↗(On Diff #120053)

popping that between 2 public blocks seems unnecessary. Could checkResourceLimit be moved to the public: block at the beginning of SchedBoundary? Also, what's the reasoning behind making updateIsResourceLimited the only protected function in SchedBoundary?

jonpa updated this revision to Diff 120075.Oct 24 2017, 7:48 AM

Thanks for review, Florian. Does this look better?

fhahn added a comment.Oct 24 2017, 9:09 AM

looks good to me, but let's see what others think ;)

fhahn added a comment.Oct 24 2017, 9:12 AM

Also, could you add a more descriptive title than minor refactoring?

jonpa retitled this revision from [MachineScheduler] minor refactoring to [MachineScheduler] minor refactoring: new static function checkResourceLimit().Oct 24 2017, 9:28 AM
MatzeB edited edge metadata.Oct 24 2017, 9:56 AM

A clear "meh" from my side :)
Feel free to commit it.

lib/CodeGen/MachineScheduler.cpp
1838
  • Use doxygen /// comments
  • Unnecessary braces around return expression
fhahn accepted this revision.Oct 24 2017, 10:38 AM

LGTM with Matthias's nits

This revision is now accepted and ready to land.Oct 24 2017, 10:38 AM
jonpa closed this revision.Oct 25 2017, 1:28 AM

r316560.