Page MenuHomePhabricator

[IPConstProp] Replace and move tests to SCCP.
ClosedPublic

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

Details

Summary

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.Thu, Jul 23, 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.Thu, Jul 23, 10:15 AM
llvm/test/Transforms/IPConstantProp/multiple_callbacks.ll
2

Missing run line?

llvm/test/Transforms/IPConstantProp/return-argument.ll
56

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 (http://lists.llvm.org/pipermail/llvm-dev/2020-July/143703.html). 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.Thu, Jul 23, 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.Fri, Jul 24, 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.Fri, Jul 24, 6:10 AM

LGTM. Assuming no one using this comes forward.

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

Thanks, so far there have been no objections on llvm-dev (http://lists.llvm.org/pipermail/llvm-dev/2020-July/143773.html) I'll land this soon. We can always bring it back again.

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