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

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban 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));
----------------
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.




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

https://reviews.llvm.org/D143487