[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:358
     // Create the new name.
-    MallocAllocator Allocator;
-    setValueName(ValueName::create(NameRef, Allocator));
-    getValueName()->setValue(this);
+    if (NeedNewName) {
+      MallocAllocator Allocator;
----------------
yrouban wrote:
> efriedma wrote:
> > This change shouldn't be necessary, I think?  If NeedNewName is false, then NameRef is the empty string, so we can't hit this codepath.
> Do you suggest that we remove this condition on NeedNewName and set the name unconditionally here?
> I agree that If NeedNewName is false then NameRef is empty. But ValueName::create() still allocates memory (it is StringMapEntry<Value*>::create()) and sets HasName = true.
> 
> In other words, we need this additional condition because we narrowed down the fast path in the beginning, so we can reach this part with NeedNewName == false.
A few lines earlier, we have:

```
    if (NameRef.empty()) {
      // Free the name for this value.
      destroyValueName();
      return;
    }
```

If NameRef is the empty string, we return early.  If NameRef isn't the empty string, NeedNewName must be true.  Either way, the additional check here does nothing.


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

https://reviews.llvm.org/D143487