[PATCH] D143487: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/IR/Value.cpp:373
   // Name is changing to something new.
-  setValueName(ST->createValueName(NameRef, this));
+  if (NeedNewName)
+    setValueName(ST->createValueName(NameRef, this));
----------------
yrouban wrote:
> efriedma wrote:
> > yrouban wrote:
> > > efriedma wrote:
> > > > Similarly here... given NameRef is the empty string, if hasName() is true, we hit:
> > > > 
> > > > ```
> > > >     if (NameRef.empty())
> > > >       return;
> > > > ```
> > > > 
> > > > if hasName() is false, we hit:
> > > > 
> > > > ```
> > > >   if (getName() == NameRef)
> > > >     return;
> > > > ```
> > > > 
> > > > So NeedNewName is always true here.
> > > Here at the line 373 we can have:
> > > 
> > > //hasName// && !NameRef.empty() && //getName// != NameRef
> > > 
> > > given that //hasName// and //getName// are calculated as //hasName()// and //getName()// before the name destruction.
> > > 
> > > In other words, the current name and the new name are not empty and not equal. It is the main path for changing an existing name. In the previous block we have just destroyed the old name and is about to set the new one. I agree that for //!hasName// we have //NeedNewName == true// but for //hasName == true//  //NeedNewName// can be false.
> > > 
> > > I'm not sure I understand your point. Do you propose a deeper refactoring like the following:
> > > 
> > > ```
> > >   ...
> > >   if (!hasName()) {
> > >     assert(NeedNewName);
> > >     setValueName(ST->createValueName(NameRef, this));
> > >     return;
> > >   }
> > > 
> > >   // Remove old name.
> > >   // NOTE: Could optimize for the case the name is shrinking to not deallocate
> > >   // then reallocated.
> > >   ST->removeValueName(getValueName());
> > >   destroyValueName();
> > > 
> > >   if (!NameRef.empty()) {
> > >     assert(NeedNewName);
> > >     setValueName(ST->createValueName(NameRef, this));
> > >   }
> > > }
> > > 
> > > ```
> > > 
> > > or a simple one
> > > 
> > > ```
> > >   ...
> > >   if (hasName()) {
> > >     // Remove old name.
> > >     // NOTE: Could optimize for the case the name is shrinking to not deallocate
> > >     // then reallocated.
> > >     ST->removeValueName(getValueName());
> > >     destroyValueName();
> > >   }
> > > 
> > >   if (NeedNewName && !NameRef.empty())
> > >     setValueName(ST->createValueName(NameRef, this));
> > > }
> > > 
> > > ```
> > > 
> > > Please suggest an exact code, if possible.
> > > but for hasName == true NeedNewName can be false.
> > 
> > If hasName is true and NeedNewName is false, NameRef is the empty string, so we hit the check on 368, and return early:
> > 
> > ```
> > if (NameRef.empty())
> >   return;
> > ```
> > 
> > So I'm suggesting the following:
> > 
> > ```
> > assert(NeedNewName);
> > setValueName(ST->createValueName(NameRef, this));
> > ```
> I still cannot agree.
> At line 373 we have the following conditions satisfied:
> 
> ```
> !(!NeedNewName && !hasName()) // came from line 323
> !(NewName.isTriviallyEmpty() && !hasName()) // came from 327
> !(getName() == NameRef) // came from 336
> !(hasName() && NewRef.empty()) // came from 368
> 
> ```
> Simplified:
> ```
> 323: NeedNewName || hasName()
> 327: !NewName.isTriviallyEmpty() || hasName()
> 336: getName() != NameRef
> 368: !hasName() || !NewRef.empty()
> ```
> Observe two cases: hasName() == true and hasName() == false
> 
> hasName() == false further simplifies to the following:
> ```
> 323: NeedNewName
> 327: !NewName.isTriviallyEmpty()
> 336: getName() != NameRef
> ```
> hasName() == true  further simplifies to the following:
> ```
> 336: getName() != NameRef
> 368: !NewRef.empty()
> ```
> So, for the case hasName() == true we do not have any restriction on NeedNewName. Thus it can be false and wee need if (NeedNewName) at line 373.
> 
> 
!NameRef.empty() means NameRef isn't the empty string, which means NeedNewName is true (because if NeedNewName were false, we would have set NameRef to the empty string).  So no, it can't be false.


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

https://reviews.llvm.org/D143487