[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:
> > 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));
```


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

https://reviews.llvm.org/D143487