[PATCH] D73342: Fix EarlyCSE to intersect aliasing metadata.

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D73342#1841992 <https://reviews.llvm.org/D73342#1841992>, @sheredom wrote:

> In D73342#1839002 <https://reviews.llvm.org/D73342#1839002>, @hfinkel wrote:
>
> > In D73342#1838953 <https://reviews.llvm.org/D73342#1838953>, @nikic wrote:
> >
> > > Can you explain in more detail why the current behavior is incorrect? Why is using the aliasing information in program order wrong? If the first load is noalias, then we should be able to deduplicate to that noalias load. If the second load is noalias, then generally we can't assume the first load is also noalias. Unless there is a must-exec relation between them, as @nlopes mentioned.
> >
> >
> > We have two issues here;
> >
> > 1. For any pass, if it changes any instruction with metadata that it does not understand, it should drop that metadata. That's the contract that we need to maintain.
> > 2. For this AA metadata in particular, we need to intersect the metadata, and this is true to preserve semantics for almost all other known metadata (AA, profiling, fp-precision, debug) although likely for it is not correct to keep only one, or the other, because of potential dynamic dependencies on the results.
> >
> >   For AA, as an example, we can have something like this:
> >
> >   int * restrict r = a; int x = should_alias ? *a : *r;
> >
> >   (essentially the same is true using casts as TBAA).
> >
> >   In any case, we should be calling the function llvm::combineMetadataForCSE here. Please update the patch to do that.
>
>
> So when I use combineMetadataForCSE it breaks the invariant loads tests because the code there just says 'if one thing is invariant load and the other is not, throw the invariant load metadata away'.
>
> What would be the correct fix for this? I don't know a ton about the intracacies of invariant loads.


I believe that invariant loads are a bit special because the invariantness of a load is not allowed to have any control dependencies. The LangRef says, "If a load instruction tagged with the !invariant.load metadata is executed, the optimizer may assume the memory location referenced by the load contains the same value at all points in the program where the memory location is known to be dereferenceable; otherwise, the behavior is undefined." Thus, I think that it's valid for (invariant + nothing) to be (invariant) so long as both are guaranteed to be executed.

I would, however, do this as two separate patches. One which updates EarlyCSE to use combineMetadataForCSE (and XFAILs the test), and a second which proposes to update combineMetadataForCSE and fixes the test (and then we can figure out in that review what to do, because we may need a dominance check or a guaranteed-to-execute check or similar).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73342/new/

https://reviews.llvm.org/D73342