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

Mon Mar 20 12:50:51 PDT 2023

efriedma added a comment.

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.

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.  That's mostly equivalent to this patch, but saves a bunch of conditional checks.  That should be a performance improvement whether or not shouldDiscardValueNames() is enabled... although I'm not sure the improvement would be measurable.

================
Comment at: llvm/lib/IR/Value.cpp:320
-  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
-    return;
-
----------------
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.

================
Comment at: llvm/lib/IR/Value.cpp:323
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&
----------------
Can we just make this `StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";`,instead of messing with the rest of the function?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143487