This is an archive of the discontinued LLVM Phabricator instance.

[Dominators] Add isUpdateLazy() method to the DomTreeUpdater
ClosedPublic

Authored by NutshellySima on Jul 7 2018, 10:32 PM.

Details

Summary

Previously, when people need to deal with DTU with different UpdateStrategy using different actions, they need to

if (DTU.getUpdateStrategy() == DomTreeUpdater::UpdateStrategy::Lazy) {
  ...
}
if (DTU.getUpdateStrategy() == DomTreeUpdater::UpdateStrategy::Eager) {
  ...
}

After the patch, they can avoid code patterns above

if (DTU.isUpdateLazy()){
  ...
}
if (!DTU.isUpdateLazy()){
  ...
}

Diff Detail

Event Timeline

NutshellySima created this revision.Jul 7 2018, 10:32 PM
kuhar added a comment.Jul 9 2018, 8:29 AM

Perhaps it would make sense to expose a similar function for the Eager strategy?
And I would make it sorter, so just isLazy and isEager -- shouldn't make anything more ambiguous IMO.

include/llvm/IR/DomTreeUpdater.h
49

Maybe something shorter would also fit here, like:
// Returns true if the current strategy is Lazy.?

Perhaps it would make sense to expose a similar function for the Eager strategy?
And I would make it sorter, so just isLazy and isEager -- shouldn't make anything more ambiguous IMO.

+1 to both. Having both options makes it easier to read the if checks in my opinion. Otherwise LGTM.

Perhaps it would make sense to expose a similar function for the Eager strategy?
And I would make it sorter, so just isLazy and isEager -- shouldn't make anything more ambiguous IMO.

+1 to both. Having both options makes it easier to read the if checks in my opinion. Otherwise LGTM.

+1. B.T.W, I removed the getUpdateStrategy function because isLazy and isEager can replace its usage.

Address comments.

NutshellySima marked an inline comment as done.Jul 11 2018, 8:43 AM
kuhar accepted this revision.Jul 11 2018, 8:51 AM

Looks like a nice improvement!

This revision is now accepted and ready to land.Jul 11 2018, 8:51 AM
This revision was automatically updated to reflect the committed changes.