[PATCH] D87778: [MemorySSA] Be more conservative when traversing MemoryPhis.
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 17 13:18:29 PDT 2020
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
More information about the llvm-commits
mailing list