This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] Fix handling of aliasing metadata.
ClosedPublic

Authored by eli.friedman on Jun 17 2016, 12:24 AM.

Details

Summary

The correctness fix here is that when we CSE a load with another load,
we need to combine the metadata on the two loads. This matches the
behavior of other passes, like instcombine and GVN.

There's also a minor optimization improvement here: for load PRE, the
aliasing metadata on the inserted load should be the same as the
metadata on the original load. Not sure why the old code was throwing
it away.

Issue found by inspection.

Diff Detail

Repository
rL LLVM

Event Timeline

eli.friedman retitled this revision from to [JumpThreading] Fix handling of aliasing metadata..
eli.friedman updated this object.
eli.friedman added subscribers: llvm-commits, yuyichao.
hiraditya added inline comments.
lib/Transforms/Scalar/JumpThreading.cpp
965 ↗(On Diff #61067)

The declaration of KnownIDs seem to be repeated at other places e.g., line 827. Is there a way to merge them, possibly put them all in a separate function.

It should be possible to merge it together... actually, that's probably a good idea considering the number of different versions we have of essentially the same code. GVN has a version (and is missing checks in a couple places), Instcombine has two versions, EarlyCSE has a version (and is missing checks in a couple of places), BBVectorize has a version. Basically, it's a mess. We probably want a general "CSE-metadata" utility. (There's probably also room for a "hoist-metadata" utility.)

That said, I'm not sure it makes sense to do that in this patch.

hfinkel added inline comments.Jul 24 2016, 3:02 PM
lib/Transforms/Scalar/JumpThreading.cpp
965 ↗(On Diff #61067)

I agree; we'll now have this list of 'metadata which is safe to merge on memory-access CSE'. Let's put this list somewhere common.

975 ↗(On Diff #61067)

Is AATags now dead?

eli.friedman added inline comments.Jul 24 2016, 3:22 PM
lib/Transforms/Scalar/JumpThreading.cpp
965 ↗(On Diff #61067)

Okay; should I add a new utility method called "combineMetadataForMemoryCSE", or do you have some other suggestion?

975 ↗(On Diff #61067)

Not quite; it's used for inserting a new load instruction. See "if (UnavailablePred)..." around line 1073.

hfinkel accepted this revision.Jul 24 2016, 3:29 PM
hfinkel edited edge metadata.
hfinkel added inline comments.
lib/Transforms/Scalar/JumpThreading.cpp
965 ↗(On Diff #61067)

SGTM; and with that, this LGTM.

This revision is now accepted and ready to land.Jul 24 2016, 3:29 PM
This revision was automatically updated to reflect the committed changes.