Page MenuHomePhabricator

[SCCP] Add support for insertelement instructions
AbandonedPublic

Authored by davide on Jul 8 2016, 7:25 PM.

Details

Reviewers
eli.friedman
Summary

The support was already there, but it was commented out, and didn't get some cases right, e.g. the one where either idx or elt being undef.

Diff Detail

Event Timeline

davide updated this revision to Diff 63377.Jul 8 2016, 7:25 PM
davide retitled this revision from to [SCCP] Add support for insertelement instructions.
davide updated this object.
davide added a reviewer: eli.friedman.
davide added a subscriber: llvm-commits.

Also, added test cases for all these cases (at least the one I thought of)

eli.friedman edited edge metadata.Jul 9 2016, 3:19 PM

I think you're misunderstanding what "isUndefined" means... it means "we haven't computed the value of this instruction yet", not "this value is an UndefValue". See https://www.cs.rice.edu/~keith/512/2011/Lectures/L19-SCCP-1up.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.105.4146&rep=rep1&type=pdf .

davide added a comment.Jul 9 2016, 3:26 PM

I think you're misunderstanding what "isUndefined" means... it means "we haven't computed the value of this instruction yet", not "this value is an UndefValue". See https://www.cs.rice.edu/~keith/512/2011/Lectures/L19-SCCP-1up.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.105.4146&rep=rep1&type=pdf .

Yes, I was confused.. the code actually in some places uses isUndefined() as synonim to undef which now I see why it's completely wrong... I'd like to rename isUndefined() to isUnknown(), which is what Keith Cooper uses in the slides, what do you think?

Sure, that's fine (in a separate commit, of course).

davide added a comment.Jul 9 2016, 3:57 PM

I think you're misunderstanding what "isUndefined" means... it means "we haven't computed the value of this instruction yet", not "this value is an UndefValue". See https://www.cs.rice.edu/~keith/512/2011/Lectures/L19-SCCP-1up.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.105.4146&rep=rep1&type=pdf .

Yes, I was confused.. the code actually in some places uses isUndefined() as synonim to undef which now I see why it's completely wrong... I'd like to rename isUndefined() to isUnknown(), which is what Keith Cooper uses in the slides, what do you think?

Sure, that's fine (in a separate commit, of course).

I also went forward and removed all the dead vector code b38095fbc8454f96e9d4ff88db28769cba741f71 which used isUndefined() wrongly

davide abandoned this revision.Dec 5 2016, 7:15 AM