Migrate away from Value::getName()

[lattner]

Bugzilla Link	2162
Resolution	FIXED
Resolved on	Aug 24, 2009 01:43
Version	1.0
OS	All
CC	@asl,@isanbard,@nlewycky

Extended Description

Internally value names are stored in a StringMap, which is an efficient data structure for finding and uniquing and autorenaming names that are in the symbol table. This is goodness and it's FAR more efficient than something like std::map<std::string, Value*> which is what we used to use.

The disadvantage of this is that the map doesn't hold an std::string, which means that (in particular) Value::getName() is extremely expensive. It is expensive because it copies the name of the value into an std::string to return it. This means that all clients of it that matter should move off of it to reduce our compile times. This will significantly reduce heap thrashing.

There are many ways to improve the situation. For example:

1. This like that asmwriter that need to print out the string can move to using Value::getNameStart()+Value::getNameLen(), which are both very efficient.

2. The nasty std:string Tmp = V1->getName(); V1->setName(""); V2->setName(Tmp) idiom can be replaced with the far more efficient and nicer V2->takeName(V1) call.

3. I common pattern is copy the name of some value with a suffix. We should add a helper to value for this, something like V1->copySuffixedName(V2, "i") or something, which adds ".i".

4. It is also very common to use a numeric suffix, with this pattern: V2->setName(V1->getName()+"."+utostr(i)) This is expensive for many reasons: utostr allocates a string, the concatenations do as well, etc. This is generally horrible for memory use and performance. It would be better to add a V1->copySuffixedName(V2, i) to handle this, and implement it efficiently.

5. setName(std::string) is also fairly expensive if the input is not a string. Clients should start using the setName version that takes a const char* + length for full generality, or just an const char* if they don't care about nul's in the string.

6. Any uses of getName() that occur in debug code or in other places that we don't care about should migrate over to using Value::getNameStr(). This will allow us to identify places that we haven't look at yet.

When the full audit + migration is done, we can remove Value::getName() entirely.

-Chris

[llvmbot]

Almost done...

[lattner]

#​4 is left.

[lattner]

I think this is done now? Daniel?

[llvmbot]

Totally! The magic behavior to propagate null names isn't used, but this is only a nice-to-have, for a rainy day.