[IR] Allow destruction of symbol table entries regardless of DiscardValueNames

[yrouban]

Summary

Value::setNameImpl() is used both to set and reset name of the value.
In destructor of Function all arguments get reset their names (see Function::clearArguments()).
If the arguments had their names set (e.g. when the function was created with LLVMContex::DiscardValueNames == true) then their ValueName entries referred by the function's symbol table must be destructed. They are not destructed if LLVMContex::DiscardValueNames is set to false because of the fast path in Value::setNameImpl(). See the new test cases that demonstrate the problem. Without the fix they both crash in the function's destructor.

In Value::setNameImpl() this patch narrows down the fast path return for DiscardValueNames == true to allow destruction of ValueName entries if any.

[arsenm]

This is only an issue if shouldDiscardValueNames() changes mid-compilation? What’s the use case for that?

llvm/unittests/IR/BasicBlockTest.cpp

549

This needs to be ASSERT_

554

Ditto

[yrouban]

In D143487#4109813, @arsenm wrote:

This is only an issue if shouldDiscardValueNames() changes mid-compilation? What’s the use case for that?

We use one context for many subsequent compilations. Modules are removed and created as needed. In product mode DiscardValueNames are set to false. Sometimes we inject code using parseAssemblyString(), for this we have to set DiscardValueNames to true.

[yrouban]

@efriedma, @jsilvanus, @tejohnson, @arsenm, please review or suggest someone who can.

[efriedma]

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.

llvm/lib/IR/Value.cpp

320

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.

331

Can we just make this StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";,instead of messing with the rest of the function?

[yrouban]

In 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.

llvm/lib/IR/Value.cpp

320

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.

331

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.

[efriedma]

llvm/lib/IR/Value.cpp

331

Yes, D143550 does that... but it also rearranges the whole function in a bunch of other ways that I'm not sure make sense. I'd prefer the minimal change for the bugfix, then considering the full refactoring separately.

[yrouban]

Returned back the first fast pass with hasName() added to the condition as suggested.

[efriedma]

llvm/lib/IR/Value.cpp

358

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.

[yrouban]

nothing new. just added the full context lines

[yrouban]

llvm/lib/IR/Value.cpp

358

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.

[efriedma]

llvm/lib/IR/Value.cpp

358

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.

[yrouban]

llvm/lib/IR/Value.cpp

358

now I see. Let me restructure the code a bit further ...

[yrouban]

made the (!ST) case simpler

[efriedma]

llvm/lib/IR/Value.cpp

374

Similarly here... given NameRef is the empty string, if hasName() is true, we hit:

if (NameRef.empty())
  return;

if hasName() is false, we hit:

if (getName() == NameRef)
  return;

So NeedNewName is always true here.

[yrouban]

llvm/lib/IR/Value.cpp

374

Here at the line 373 we can have:

hasName && !NameRef.empty() && getName != NameRef

given that hasName and getName are calculated as hasName() and getName() before the name destruction.

In other words, the current name and the new name are not empty and not equal. It is the main path for changing an existing name. In the previous block we have just destroyed the old name and is about to set the new one. I agree that for !hasName we have NeedNewName == true but for hasName == true NeedNewName can be false.

I'm not sure I understand your point. Do you propose a deeper refactoring like the following:

  ...
  if (!hasName()) {
    assert(NeedNewName);
    setValueName(ST->createValueName(NameRef, this));
    return;
  }

  // Remove old name.
  // NOTE: Could optimize for the case the name is shrinking to not deallocate
  // then reallocated.
  ST->removeValueName(getValueName());
  destroyValueName();

  if (!NameRef.empty()) {
    assert(NeedNewName);
    setValueName(ST->createValueName(NameRef, this));
  }
}

or a simple one

  ...
  if (hasName()) {
    // Remove old name.
    // NOTE: Could optimize for the case the name is shrinking to not deallocate
    // then reallocated.
    ST->removeValueName(getValueName());
    destroyValueName();
  }

  if (NeedNewName && !NameRef.empty())
    setValueName(ST->createValueName(NameRef, this));
}

Please suggest an exact code, if possible.

[efriedma]

llvm/lib/IR/Value.cpp

374

but for hasName == true NeedNewName can be false.

If hasName is true and NeedNewName is false, NameRef is the empty string, so we hit the check on 368, and return early:

if (NameRef.empty())
  return;

So I'm suggesting the following:

assert(NeedNewName);
setValueName(ST->createValueName(NameRef, this));

[yrouban]

llvm/lib/IR/Value.cpp

374

I still cannot agree.
At line 373 we have the following conditions satisfied:

!(!NeedNewName && !hasName()) // came from line 323
!(NewName.isTriviallyEmpty() && !hasName()) // came from 327
!(getName() == NameRef) // came from 336
!(hasName() && NewRef.empty()) // came from 368

Simplified:

323: NeedNewName || hasName()
327: !NewName.isTriviallyEmpty() || hasName()
336: getName() != NameRef
368: !hasName() || !NewRef.empty()

Observe two cases: hasName() == true and hasName() == false

hasName() == false further simplifies to the following:

323: NeedNewName
327: !NewName.isTriviallyEmpty()
336: getName() != NameRef

hasName() == true further simplifies to the following:

336: getName() != NameRef
368: !NewRef.empty()

So, for the case hasName() == true we do not have any restriction on NeedNewName. Thus it can be false and wee need if (NeedNewName) at line 373.

[efriedma]

llvm/lib/IR/Value.cpp

374

!NameRef.empty() means NameRef isn't the empty string, which means NeedNewName is true (because if NeedNewName were false, we would have set NameRef to the empty string). So no, it can't be false.

[yrouban]

llvm/lib/IR/Value.cpp

374

Ok, I see now that I missed the name at line 368: it is about NameRef rather than NewRef.

[yrouban]

replaced

if (NeedNewName)

with

assert(NeedNewName);

at line 373.

[yrouban]

@efriedma, please take a look at the latest change you suggested.

[efriedma]

LGTM

llvm/lib/IR/Value.cpp

StringRef Value::getName() const {
   // some clients want to call .data() on the result and expect it to be null
   // terminated.
   if (!hasName())
     return StringRef("", 0);
   return getValueName()->getKey();
 }
 
 void Value::setNameImpl(const Twine &NewName) {
+  bool NeedNewName =
+      !getContext().shouldDiscardValueNames() || isa<GlobalValue>(this);
+
   // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
-  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
+  // and there is no need to delete the old name.
+  if (!NeedNewName && !hasName())
     return;
 
   // Fast path for common IRBuilder case of setName("") when there is no name.
   if (NewName.isTriviallyEmpty() && !hasName())
     return;
 
   SmallString<256> NameData;
-  StringRef NameRef = NewName.toStringRef(NameData);
+  StringRef NameRef = NeedNewName ? NewName.toStringRef(NameData) : "";
   assert(NameRef.find_first_of(0) == StringRef::npos &&
          "Null bytes are not allowed in names");
 
   // Name isn't changing?
   if (getName() == NameRef)
     return;
 
   assert(!getType()->isVoidTy() && "Cannot assign a name to void values!");
 
   // Get the symbol table to update for this object.
   ValueSymbolTable *ST;
   if (getSymTab(this, ST))
     return;  // Cannot set a name on this value (e.g. constant).
 
   if (!ST) { // No symbol table to update?  Just do the change.
-    if (NameRef.empty()) {
-      // Free the name for this value.
-      destroyValueName();
-      return;
-    }
-
     // NOTE: Could optimize for the case the name is shrinking to not deallocate
     // then reallocated.
     destroyValueName();
 
+    if (!NameRef.empty()) {
       // Create the new name.
+      assert(NeedNewName);
       MallocAllocator Allocator;
       setValueName(ValueName::create(NameRef, Allocator));
       getValueName()->setValue(this);
+    }
     return;
   }
 
   // NOTE: Could optimize for the case the name is shrinking to not deallocate
   // then reallocated.
   if (hasName()) {
     // Remove old name.
     ST->removeValueName(getValueName());
     destroyValueName();
 
     if (NameRef.empty())
       return;
   }
 
   // Name is changing to something new.
+  assert(NeedNewName);
   setValueName(ST->createValueName(NameRef, this));
 }
 
 void Value::setName(const Twine &NewName) {
   setNameImpl(NewName);
   if (Function *F = dyn_cast<Function>(this))
     F->recalculateIntrinsicID();
 }
 

llvm/unittests/IR/BasicBlockTest.cpp

)");
   BB0->erase(Instr1->getIterator(), Instr2->getIterator());
   EXPECT_EQ(BB0->size(), 2u);
   EXPECT_EQ(&*BB0->begin(), Instr2);
 
   // Erase all instructions
   BB0->erase(BB0->begin(), BB0->end());
   EXPECT_TRUE(BB0->empty());
 }
+
+TEST(BasicBlockTest, DiscardValueNames) {
+  const char *ModuleString = "declare void @f(i32 %dangling)";
+  SMDiagnostic Err;
+  LLVMContext Ctx;
+  { // Scope of M.
+    auto M = parseAssemblyString(ModuleString, Err, Ctx);
+    ASSERT_TRUE(M.get());
+    EXPECT_FALSE(Ctx.shouldDiscardValueNames());
+  }
+  { // Scope of M.
+    auto M = parseAssemblyString(ModuleString, Err, Ctx);
+    ASSERT_TRUE(M.get());
+    Ctx.setDiscardValueNames(true);
+  }
+}
+
+TEST(BasicBlockTest, DiscardValueNames2) {
+  SMDiagnostic Err;
+  LLVMContext Ctx;
+  Module M("Mod", Ctx);
+  auto FTy = FunctionType::get(Type::getVoidTy(M.getContext()),
+                               {Type::getInt32Ty(Ctx)}, /*isVarArg=*/false);
+  { // Scope of F.
+    Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", &M);
+    F->getArg(0)->setName("dangling");
+    F->removeFromParent();
+    EXPECT_FALSE(Ctx.shouldDiscardValueNames());
+    delete F;
+  }
+  { // Scope of F.
+    Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", &M);
+    F->getArg(0)->setName("dangling");
+    F->removeFromParent();
+    Ctx.setDiscardValueNames(true);
+    delete F;
+  }
+}
+
 } // End anonymous namespace.
 } // End llvm namespace.