[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());
----------------
asbirlea wrote:
> 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?
> 
> I *think* this is conservatively correct. What do you think?

Thanks, I applied the suggestion. This should be conservatively correct. Once this goes in I plan on enabling DSE + MemorySSA again and see if/what the remaining failures are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87778