This is an archive of the discontinued LLVM Phabricator instance.

[IPSCCP] Move common functions to IPOUtils (NFC)
ClosedPublic

Authored by mssimpso on Sep 8 2017, 12:26 PM.

Details

Summary

This patch moves some common utility functions out of IPSCCP and makes them available globally. The functions determine if interprocedural data-flow analyses can propagate information through function returns, arguments, and global variables.

This refactoring was suggested in the review of D37335.

Diff Detail

Repository
rL LLVM

Event Timeline

mssimpso created this revision.Sep 8 2017, 12:26 PM
mssimpso updated this revision to Diff 115416.Sep 15 2017, 9:57 AM
mssimpso edited the summary of this revision. (Show Details)

Some updates.

  • I moved the functions over to Analysis. This makes more sense to me since they don't modify the code at all. They'll also be able to be used by other analyses that way.
  • I added back the removeDeadConstantUsers call in IPSCCP. This was an omission from the last version of the patch.

So, i can't find what the suggestion was, but these are fairly specific to value lattices and don't belong being used by most other things.
There are lattices where they are correct, and lattices where they make no sense.
It's also kinda conservative.

IMHO, these aren't generic enough to want to be used by a lot other things (plus at least one of them would lead to N^2 usage).
They definitely don't belong in something called "IPOUtils", because it implies they should be used broadly.

If you renamed it to ValueLatticeUtils or something, i'd be okay with it.

lib/Analysis/IPOUtils.cpp
32 ↗(On Diff #115416)

this is just any_of.

It's also super-conservative.

mssimpso updated this revision to Diff 118447.Oct 10 2017, 12:03 PM

Addressed Danny's comments.

  • Rename to ValueLatticeUtils
  • Use any_of
  • I'll retitle the patch when committing to avoid starting a new thread on llvm-commits.

I think Davide originally suggested moving these helpers out of IPSCCP. I had essentially duplicated them for the CalledValuePropagation pass, and the idea was to eliminate the duplication. However, I do recall that we were unsure of the broader usefulness of the global variables function in particular. I think renaming to ValueLatticeUtils helps address this, though.

dberlin accepted this revision.Oct 10 2017, 10:38 PM
This revision is now accepted and ready to land.Oct 10 2017, 10:38 PM
This revision was automatically updated to reflect the committed changes.