[PATCH] D87778: [MemorySSA] Be more conservative when traversing MemoryPhis.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/MemorySSA.h:1262
         if (Translator.getAddr() != Location.Ptr) {
-          CurrentPair.second = Location.getWithNewPtr(Translator.getAddr());
+          CurrentPair.second = Location.getWithNewPtr(Translator.getAddr())
+                                   .getWithNewSize(LocationSize::unknown());
----------------
fhahn wrote:
> asbirlea wrote:
> > I'm not clear that the size should be always unknown after phi translation with different ptr, but feel free to correct me.
> > Here's my suggestion.
> > ```
> > if (Translator.getAddr() != Location.Ptr) {
> >   CurrentPair.second = CurrentPair.second.getWithNewPtr(Translator.getAddr());
> >   if (PerformedPhiTranslation)
> >             *PerformedPhiTranslation = true;
> > } else {
> >   CurrentPair.second = Location.getWithNewSize(LocationSize::unknown());
> > }
> > ```
> Hm, I think if we translate to a different address, we still should set to an unknown size, as the translated address could itself be loop dependent and reference multiple actual locations.
> 
> But if we translate to an invariant address, it should probably be safe to use the precise address?
> 
> The invariance criteria in the patch is just a few things that matter in practice and are easy to check, but I am sure there is potential for detecting more cases there, after we established a safe baseline
I tried to come up with a test case where things go wrong when we use the precise location after successfully translating the address, but failed to do so.

I adjusted the code to keep the precise location when PHI translation succeeded. I am still not entirely sure if this is completely safe, but with DSE & MemorySSA there is now a client that will most likely surface a test case, if things go wrong.

Please let me know which approach you prefer, I am fine with either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87778