[PATCH] D21271: Fix `InstCombine` to not widen metadata on store-to-load forwarding

[Eli Friedman via llvm-commits]

eli.friedman accepted this revision.
eli.friedman added a reviewer: eli.friedman.
eli.friedman added a comment.
This revision is now accepted and ready to land.

LGTM.

----

In http://reviews.llvm.org/D21271#455734, @yuyichao wrote:

> Does it mean that if that store is not merged with something else in a similar way it can be incorrectly optimized?


Not sure what the question is here.  You don't need to touch the metadata in store->load forwarding: you're erasing the load, and the semantics of the store aren't affected by a subsequent load.

In http://reviews.llvm.org/D21271#455735, @yuyichao wrote:

> > would you mind fixing JumpThreading as well?
>
>
> I assume you mean the use of this function in JumpThreading? I did a grep before submitting this and didn't see that one having similar problem since it is not conditioning on the type of the returned instruction (not in an obvious way for me anyway). I'm willing to fix it if I can understand what's the problem.


Well, the primary issue in JumpThreading is that it doesn't strip the load metadata using combineMetadata or something like that where appropriate.  It also uses the metadata from the store in store->load forwarding in some cases, sort of the opposite of the instcombine issue.  Taking another look, it's more than just a few lines to fix, so probably best to submit for review separately.


http://reviews.llvm.org/D21271