Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[IPConstProp] Replace and move tests to SCCP.

Authored by fhahn on Jul 23 2020, 9:58 AM.



As far as I know, ipconstprop has not been used in years and ipsccp has
been used instead. This has the potential for confusion and sometimes
leads people to spend time finding & reporting bugs as well as
updating it to work with the latest API changes.

This patch moves the tests over to SCCP. There's one functional difference
I am aware of: ipconstprop propagates for each call-site individually, so
for functions that are called with different constant arguments it can sometimes
produce better results than ipsccp (at much higher compile-time cost).But
IPSCCP can be thought to do so as well for internal functions and as mentioned
earlier, the pass seems unused in practice (and there are no plans on working
towards enabling it anytime).

Diff Detail

Event Timeline

fhahn created this revision.Jul 23 2020, 9:58 AM
Herald added a project: Restricted Project. · View Herald Transcript

Maybe ask llvm community on mailing list for permission to remove it from codebase?

xbolva00 added inline comments.Jul 23 2020, 10:15 AM

Missing run line?


It would be useful to handle this too

Maybe ask llvm community on mailing list for permission to remove it from codebase?

I replied to a thread with that suggestion ( Let's see if there are any obvious things I am missing. Happy to send a heads-up to the list after that.

FWIW, I'm in favor of deleting this.

nikic added a subscriber: nikic.Jul 23 2020, 10:48 AM

It looks like a lot of the tests here are about !callback support.

It looks like a lot of the tests here are about !callback support.

Yes. Pre-Attributor callbacks were only supported by this pass. These tests are subsumed by the Attributor test and Attributor interprets !callback. That said, removing those tests for SCCP is fine with me. Keeping them as "negative" test is as well.

fhahn updated this revision to Diff 280420.Jul 24 2020, 5:26 AM

*actually* move the tests to the llvm/test/Transforms/SCCP.

As mentioned in the comments, there are 2 things IPSCCP does not support:

  • !callback metadata - that should be straight forward to add (basically switching to abstract call site?)
  • ipconstprop solves for each call site separately, so there can be more accurate results for callsites with constant arguments, at cost of (a lot of) compile-time. We could also do something similar in IPSCCP, but have to evaluate if the benefit warrants to extra compile-time. A quick sketch D84521

However I don't think any of the above should block removing ipconstprop, unless there are some real users.

jdoerfert accepted this revision.Jul 24 2020, 6:10 AM

LGTM. Assuming no one using this comes forward.

This revision is now accepted and ready to land.Jul 24 2020, 6:10 AM
fhahn edited the summary of this revision. (Show Details)Jul 27 2020, 3:38 AM
fhahn added a comment.Jul 30 2020, 2:23 AM

Thanks, so far there have been no objections on llvm-dev ( I'll land this soon. We can always bring it back again.

This revision was landed with ongoing or failed builds.Jul 30 2020, 4:36 AM
This revision was automatically updated to reflect the committed changes.

It was a hindsight, but moving the tests/deleting the pass could have been done with separate patches.