[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:
> 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.


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

https://reviews.llvm.org/D143487