[PATCH] D143487: [IR] Allow destruction of symbol table entries regardless of DiscardValueNames
Eli Friedman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 24 10:44:39 PDT 2023
efriedma added inline comments.
================
Comment at: llvm/lib/IR/Value.cpp:373
// Name is changing to something new.
- setValueName(ST->createValueName(NameRef, this));
+ if (NeedNewName)
+ setValueName(ST->createValueName(NameRef, this));
----------------
yrouban wrote:
> efriedma wrote:
> > 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.
> 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.
> 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));
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143487/new/
https://reviews.llvm.org/D143487
More information about the llvm-commits
mailing list