[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:358
     // Create the new name.
-    MallocAllocator Allocator;
-    setValueName(ValueName::create(NameRef, Allocator));
-    getValueName()->setValue(this);
+    if (NeedNewName) {
+      MallocAllocator Allocator;
----------------
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.


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

https://reviews.llvm.org/D143487