[PATCH] D143487: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames

[Yevgeny Rouban via Phabricator via llvm-commits]

yrouban added a comment.

In D143487#4207263 <https://reviews.llvm.org/D143487#4207263>, @efriedma wrote:

> If I'm understanding correctly, the issue here is that you create a context with shouldDiscardValueNames() disabled, parse some code, then enable shouldDiscardValueNames()?  That's a little exotic, but I guess we can accommodate if there isn't a measurable penalty for normal frontends.

I believe you get it right. It is a functional issue and I do not expect any performance impact.

> As an alternative to this patch, I guess we could add a "clearValueName()" method that doesn't check shouldDiscardValueNames() at all, and use it in the necessary places.

To not complicate things further I would suggest trying the //clearValueName()// approach only if we see any performance impact.



================
Comment at: llvm/lib/IR/Value.cpp:320
-  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
-    return;
-
----------------
efriedma wrote:
> I would guess it's important that we preserve the fast-path here (although I don't have numbers).  Amending it to `getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this) && !hasName())` or something like that is probably trivial enough that it doesn't really matter, but the current patch runs a lot of code that doesn't do anything.
I agree. //shouldDiscardValueNames// should be used only to check if we need the new name, but the old name (if any) we have to discard unconditionally.


================
Comment at: llvm/lib/IR/Value.cpp:323
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&
----------------
efriedma wrote:
> Can we just make this `StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";`,instead of messing with the rest of the function?
I agree that we have to avoid //NewName.toStringRef(NameData)//. I have another patch that refactors //Value::setNameImpl()// further in this way - D143550. Please take a look and choose one patch that I should finalize.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143487