[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:
> 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.
now I see. Let me restructure the code a bit further ...


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

https://reviews.llvm.org/D143487