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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea 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:
> 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.
My inclination here after further review of the code is to not set the size to unknown for all cases, but it's possible it's necessary.
I'd use the same loop invariant condition on the new address. 
```
if (Translator.getAddr() != Location.Ptr) {
  CurrentPair.second = CurrentPair.second.getWithNewPtr(Translator.getAddr());

  if (Translator.getAddr() && 
      !IsGuaranteedLoopInvariant(const_cast<Value *>(Translator.getAddr()))
          CurrentPair.second = CurrentPair.second.getWithNewSize(LocationSize::unknown());

  if (PerformedPhiTranslation)
          *PerformedPhiTranslation = true;
}
```

(Move Translator.getAddr() to a local variable)

Also, use `CurrentPair.second = CurrentPair.second.get...`, not `CurrentPair.second = Location.get` so if the size was reset in the above condition checking loop invariance, that info is kept.


I *think* this is conservatively correct. What do you think?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87778