This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Replace structs with constants if all the lattice values are constant
ClosedPublic

Authored by davide on Jul 12 2016, 10:44 AM.

Details

Summary

I originally started working on adding support for vectors insertelement and friends than I realized we could re-use the code we use for handling structs for the purpose.
So, I decided to struct support first and put the rewrite of http://reviews.llvm.org/D22186 on hold.
This patch only works for SCCP but it shouldn't be hard to port to the interprocedural version of this pass if we agree this is the right way of handling this case.

Diff Detail

Event Timeline

davide updated this revision to Diff 63693.Jul 12 2016, 10:44 AM
davide retitled this revision from to [SCCP] Replace structs with constants if all the lattice values are constant.
davide updated this object.
davide added a reviewer: eli.friedman.
davide added a subscriber: llvm-commits.
eli.friedman edited edge metadata.Jul 12 2016, 11:39 AM

You probably want to refactor this if you're planning to use it from IPSCCP... but you can do that in the followup, I guess.

In terms of expanding the struct code for vectors, I guess that might work? It's probably a lot of work for not much benefit, though; it would involve touching every single operation which can work on vectors (including basically every arithmetic operator).

test/Transforms/SCCP/constant-struct.ll
33

A little bit more test coverage would be nice... maybe a case which exercise the isOverdefined() path, and a case where the initial value is an argument or something else defined?

davide marked an inline comment as done.Jul 12 2016, 12:16 PM

You probably want to refactor this if you're planning to use it from IPSCCP... but you can do that in the followup, I guess.

Yes, I'll refactor separately.

In terms of expanding the struct code for vectors, I guess that might work? It's probably a lot of work for not much benefit, though; it would involve touching every single operation which can work on vectors (including basically every arithmetic operator).

I'm still unsure as well. Now that I have a slightly better understanding of the pass I might try that and see where it goes. If it's too complicated I'll just avoid doing that.

davide updated this revision to Diff 63709.Jul 12 2016, 12:17 PM
davide edited edge metadata.

Improved test coverage as suggested by Eli

eli.friedman accepted this revision.Jul 12 2016, 12:55 PM
eli.friedman edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jul 12 2016, 12:55 PM
This revision was automatically updated to reflect the committed changes.
reames added a subscriber: reames.Aug 31 2016, 7:14 PM

Minor post commit style comments. The patch title caught my eye so I took a quick read through.

llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
1572 ↗(On Diff #63720)

Why must it be that any non-constant must be undef? I suspect this is either a well known invariant in the code (with which I'm unfamiliar), or a possibly missing assert.

1580 ↗(On Diff #63720)

Is there a helper function which could common out this shared code?

davide added inline comments.Sep 1 2016, 9:21 AM
llvm/trunk/lib/Transforms/Scalar/SCCP.cpp
1572 ↗(On Diff #63720)

I have the algorithm out-of-cache but IIRC once the lattice solver runs there can't be values with an "undefined" lattice value, so, yes, this is an invariant maintained by the algorithm.

1580 ↗(On Diff #63720)

I tried to factor out in a later commit (a while ago).

static bool tryToReplaceWithConstant(SCCPSolver &Solver, Value *V)

Does that make sense to you?