Page MenuHomePhabricator

[IPConstantProp][NFCI] Improve and modernize tests
ClosedPublic

Authored by jdoerfert on Nov 1 2019, 11:29 PM.

Details

Summary

This change is in preparation to reuse these test for the Attributor.
It mainly is to remove UB, make it clear what is tested, and use
"modern" run lines.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 1 2019, 11:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 11:29 PM
Herald added a subscriber: bollu. · View Herald Transcript
jdoerfert updated this revision to Diff 227571.Nov 1 2019, 11:36 PM

Add two more that splipped through the cracks

jdoerfert marked 5 inline comments as done.Nov 2 2019, 12:59 PM
jdoerfert added inline comments.
llvm/test/Transforms/IPConstantProp/2009-09-24-byval-ptr.ll
27

This matches closer the check lines above and it also does not force us to fold the gep into the load (which is not what is important here).

llvm/test/Transforms/IPConstantProp/PR26044.ll
62

Having the check lines at the end makes it hard to relate them to code so I went with a update of all lines here.

llvm/test/Transforms/IPConstantProp/arg-count-mismatch.ll
57

The calls were essentially dead and could be removed, make sure that is not the optimization we are looking for.

llvm/test/Transforms/IPConstantProp/solve-after-each-resolving-undefs-for-function.ll
18

Branch on undef is UB, I think, and even if not, this method did trivially return 10 even without resolving any constant branches. Make sure this is not the case anymore and add a "for sure UB-free" path while we are at it.

llvm/test/Transforms/IPConstantProp/user-with-multiple-uses.ll
33

These attributes are given already (and arguably wrong from a more fundamental standpoint) so it is questionable why we should check for them in the first place.

fhahn added a comment.Nov 2 2019, 1:38 PM

It seems some of those files test IPSCCP while others test the IPConstantPropagation pass. Having them both in there is a bit confusing. I think most IPSCCP tests live in test/Transforms/SCCP, which they share the code with. It might be worth to move the IPSCCP tests from IPConstantPropagation to test/Transforms/SCCP or create a new IPSCCP test directory.

And FWIW, I think we might want to consider removing the IPConstantPropagation pass, because it looks like it is not used in any pipelines and IPSCCP is more powerful.

It seems some of those files test IPSCCP while others test the IPConstantPropagation pass. Having them both in there is a bit confusing. I think most IPSCCP tests live in test/Transforms/SCCP, which they share the code with. It might be worth to move the IPSCCP tests from IPConstantPropagation to test/Transforms/SCCP or create a new IPSCCP test directory.

I don't mind either way. Do you want me to do it in this commit or do it after?

And FWIW, I think we might want to consider removing the IPConstantPropagation pass, because it looks like it is not used in any pipelines and IPSCCP is more powerful.

Agreed, see: https://reviews.llvm.org/D69656#1728773


Any comments wrt. the changes I made?

davide accepted this revision.Nov 2 2019, 2:12 PM

This patch looks good to me.

Now, on the more general discussion. There are some cases that are caught by IPCP [but not IPSCCP]. We can craft examples, if we want, but it's unclear how often they happen in practice.
Having IPCP in tree doesn't seem to be a huge maintenance burden. OTOH, the algorithm is extremely naive and the pass isn't used anywhere, so if somebody wants to remove it, I wouldn't have any objections.

Also note that the cases that are caught by IPCP and not by IPSCCP are due to the fact that IPCP isn't really an analysis, and replaces stuffs as it goes. Having a context-sensitive IPSCCP pass could alleviate some of the differences, but it's unclear whether it's worth the complexity.

This revision is now accepted and ready to land.Nov 2 2019, 2:12 PM
davide added a comment.Nov 2 2019, 2:13 PM

[if you probably replace opt -ipconstprop with opt -ipsccp in the tests you'll stumble upon the cases I pointed out.

fhahn added a comment.Nov 2 2019, 2:21 PM

This patch looks good to me.

Now, on the more general discussion. There are some cases that are caught by IPCP [but not IPSCCP]. We can craft examples, if we want, but it's unclear how often they happen in practice.
Having IPCP in tree doesn't seem to be a huge maintenance burden. OTOH, the algorithm is extremely naive and the pass isn't used anywhere, so if somebody wants to remove it, I wouldn't have any objections.

Interesting! That's unfortunate, with the pass being disabled. I'll try to take a look at those when I have a bit of time.

[if you probably replace opt -ipconstprop with opt -ipsccp in the tests you'll stumble upon the cases I pointed out.

I briefly tried this and it looks like the two main features ipconstprop has and ipsccp not are: 1) multi-return/extractvalue support, 2) callback support

This revision was automatically updated to reflect the committed changes.