This is an archive of the discontinued LLVM Phabricator instance.

Value number stores and memory states so we can detect when memory states are equivalent (IE store of same value to memory). Tests coming.
ClosedPublic

Authored by dberlin on Dec 23 2016, 4:21 PM.

Event Timeline

dberlin retitled this revision from to Value number stores and memory states so we can detect when memory states are equivalent (IE store of same value to memory). Tests coming..
dberlin added a reviewer: davide.
dberlin added a subscriber: llvm-commits.
  • Add comments and fix a small issue with processed counts.

Updating D28084: Value number stores and memory states so we can detect when memory

states are equivalent (IE store of same value to memory).
Tests coming.

dberlin added inline comments.Dec 23 2016, 4:28 PM
lib/Transforms/Scalar/NewGVN.cpp
1304

It would be nice to share this part with performSymbolicPHIEvaluation, but we use a different lookup.
Thoughts welcome.

davide edited edge metadata.Dec 24 2016, 7:50 AM

This is very nice. I think I got the latest revision (unless there's an issue with phabricator) and I see many tests in Transform/NewGVN failing, some of them hitting the assertion in updateProcessedCount().
In the meanwhile, some minor comment inline.

lib/Transforms/Scalar/NewGVN.cpp
787

Nit: Comments terminate with a full stop (here and elsewhere).

1304

I agree it's not great to have the logic in two different places, I'll think about it. Maybe we can add a FIXME in the meanwhile?

1320

I think the correct spelling is #ifndef NDEBUG unless you mean something else (I'm not entirely sure there's a DEBUG macro)

1322

This could be DEBUG(dbgs() ...)

1438–1447

Nit: the style guide suggests to uses braces around the else as well.

The failures happen only in a Debug+Assert build, FWIW.

dberlin marked 4 inline comments as done.
dberlin edited edge metadata.
  • Update for review comments, add tests, fix porting bug

Updating D28084: Value number stores and memory states so we can detect when memory

states are equivalent (IE store of same value to memory).
Tests coming.

dberlin marked 2 inline comments as done.Dec 24 2016, 2:45 PM
dberlin added inline comments.
lib/Transforms/Scalar/NewGVN.cpp
1304

I've cleaned up this version, i'll clean up the other version in a followup and see if we can merge them using a template function.

dberlin marked an inline comment as done.
  • Update for review comments, add tests

Updating D28084: Value number stores and memory states so we can detect when memory

states are equivalent (IE store of same value to memory).
Tests coming.

davide accepted this revision.Dec 25 2016, 10:06 AM
davide edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 25 2016, 10:06 AM
davide added inline comments.Dec 25 2016, 10:07 AM
lib/Transforms/Scalar/NewGVN.cpp
1304

Yeah, feel free to handle it in a follow-up.

This revision was automatically updated to reflect the committed changes.