[PATCH] D11143: [RFC] Cross Block DSE

hfinkel@anl.gov via llvm-commits llvm-commits at lists.llvm.org
Tue Aug 11 01:04:53 PDT 2015


hfinkel added a comment.

In http://reviews.llvm.org/D11143#217647, @dberlin wrote:

> Sigh, my review got drafted before, but submitted after you wrote
>  this, so it looks like a reply to this part.
>
> You are right that you could hoist this check, IFF alias returns
>  must/mayalias in cases where getmodrefinfo returns mod.
>
> Is this really guaranteed to be the case?


I think that it should be. I'm not the best with the history here, but there's nothing in the comments on the interface that suggests that there should be a difference w.r.t. ordering/volatile of the instructions, and other clients, MemoryDependenceAnalysis for example, layer on their own checks for ordering/volatile.

> They are different properties, though modrefinfo is often implemented

>  with help from alias. In our world of synchronization instructions,

>  isn't it possible for ordering to cause something to not be aliased

>  with a store (which is about the abstract memory locations) but still

>  be modified by it (due to it being a release or whatever) ?


There are certainly ordering constraints to preserve, but I don't believe that AA is the right place to enforce them. Clients can certainly bail on atomics, and should if they might disturb the ordering and can't do better, but optimizations on atomics are still possible and we don't want AA clients that can handle some optimizations on some atomics to need to work around the AA interface because it forces a conservative behavior.

So this is the wrong thread to fix this problem ;) -- but I don't think adding any new dependence on this behavior is a good thing.

> Or do we consider this aliasing in the AA interface to try to avoid

>  user confusion? :)

> 

> I haven't looked deeply enough to know if we truly have consistency

>  here. I know getModRefInfo performs checks on ordering, etc, before it

>  calls alias(), and i assumed those checks were actually necessary.

>  But, you know, not a lot of unit tests here :)


Indeed I know :)


http://reviews.llvm.org/D11143





More information about the llvm-commits mailing list