Limit size of non-GlobalValue name

[serge-sans-paille]

Summary

Otherwise, in some extreme test case, very long names are created and the compiler consumes large amount of memory. Size limit is set to a relatively high value not to disturb debugging.

[serge-sans-paille]

For the record: the original test case declared arrays one order of magnitude larger. Note sure if `making the test case memory swap` is a good test...

[eddyb]

For the record, this patch alone appears to have a significant impact on the original bug report the testcase here was derived from (via bugpoint), and while there may be a remaining underlying issue with SROA, it's valuable by itself.

[hfinkel]

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better. I'd be much happier, for example, to see a change in lib/IR/Value.cpp where we have:

void Value::setNameImpl(const Twine &NewName) {
  // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
  if (getContext().shouldDiscardValueNames() && !isa<GlobalValue>(this))
    return;

  ...

  SmallString<256> NameData;
  StringRef NameRef = NewName.toStringRef(NameData);

to have something like:

// Prevent names from becoming too long.
if (!isa<GlobalValue>(this) && NameData.size() > MaxInternalName)
  NameData.resize(MaxInternalName);

test/Transforms/SROA/register-name-too-long.ll

1 ↗

(On Diff #127145)

this etst -> This test

[serge-sans-paille]

In D41296#962215, @hfinkel wrote:

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

I thought of this, very happy you suggest we go that way. In a release build, we could activate that by default and keep the names in debug build.

[mehdi_amini]

In D41296#962279, @serge-sans-paille wrote:

In D41296#962215, @hfinkel wrote:

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

I thought of this, very happy you suggest we go that way. In a release build, we could activate that by default and keep the names in debug build.

Isn't it *already* the case?
As I understand, the suggestion is to cap the length of the names in Debug build.

[serge-sans-paille]

@mehdi_amini : correct! I'm in awe of all the thing in LLVM I don't know anything about!

[mehdi_amini]

lib/IR/Value.cpp

254 ↗

(On Diff #127929)

It is unfortunate that the Twine as already been serialized past the limit, trigger memory allocation in the SmallString. But unless Twine would be significantly revamped, I don't see how to do better.

[chandlerc]

lib/IR/Value.cpp

43 ↗

(On Diff #127929)

I have found quite long names *really* useful when debugging.

I'm very happy to have a cap, but I think that cap should be quite large. something like 1k or 4k.

254 ↗

(On Diff #127929)

Yeah, but because this is just tweaking the debug build to scale a bit better, I'm OK with this approach.

test/Transforms/SROA/register-name-too-long.ll

1 ↗

(On Diff #127929)

I don't think "used to swap" or "swap" is a good description here.

Different systems have different amounts of memory and different tradeoffs between swap etc.... When describing things, I would suggest talking about how much memory was used, not what was required to allocate it.

That said, this doesn't seem like a great test. We'll never notice if it starts failing on systems with enough memory. Also, the thing you're changing has very little to do with SROA. How about a more direct test with a super long name, and checking that when its printed back out it gets truncated?

[serge-sans-paille]

@chandlerc updated test case and size limit

[serge-sans-paille]

@chandlerc does this look good to you know?

[hfinkel]

test/Bitcode/value-with-long-name.ll

7 ↗

(On Diff #128024)

This test is a bit odd. Why not using two-character names and then check that they're not truncated when you get the max to 2 and they are truncated when you set the max to 1 (or zero).

[serge-sans-paille]

test/Bitcode/value-with-long-name.ll

7 ↗

(On Diff #128024)

I can add something like that

; RUN: opt -S %s -O2 -o - | FileCheck -check-prefix=CHECK-LONG %s

; Check we correctly cap the size of newly generated non-global values name
; Force the size to be small so that the check works on release and debug build
; RUN: opt -S %s -O2 -o - -non-global-value-max-name-size=0 | FileCheck -check-prefix=CHECK-SHORT %s
; RUN: opt -S %s -O2 -o - -non-global-value-max-name-size=1 | FileCheck -check-prefix=CHECK-SHORT %s

; CHECK-LONG: %{{[a-z]{4}[a-z]+}}
; CHECK-SHORT-NOT: %{{[a-z][a-z]+}}

define i32 @f(i32 %a, i32 %b) {
  %c = add i32 %a, %b
  %d = add i32 %c, %a
  %e = add i32 %d, %b
  ret i32 %e
}

But in that case in release build, when the name are stripped, the test will fail.

[hfinkel]

LGTM

test/Bitcode/value-with-long-name.ll

7 ↗

(On Diff #128024)

(We followed up on IRC on this; names are not stripped and test has been updated).

[aaronpuchert]

In D41296#962215, @hfinkel wrote:

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

This change has the unfortunate side effect of creating unexpected error messages in the LLParser that have been observed in software using LLVM. Reading the discussion here does not indicate that this effect was intended.

It can even lead to cases where we can't read our own output. To see how this happens, let's simulate a much smaller limit of 4. The normal output of the test case would have been (I've stripped the metadata here):

$ opt -S -O2 -o - value-with-long-name.ll
define i32 @f(i32 %a, i32 %b) local_unnamed_addr #0 {
  %reass.add = add i32 %b, %a
  %reass.mul = shl i32 %reass.add, 1
  ret i32 %reass.mul
}

Now we abbreviate both value names to reas, which then needs to be disambiguated:

$ opt -S -O2 -o trunc.ll -non-global-value-max-name-size=4 value-with-long-name.ll
define i32 @f(i32 %a, i32 %b) local_unnamed_addr #0 {
  %reas = add i32 %b, %a
  %reas2 = shl i32 %reas, 1
  ret i32 %reas2
}

Now let's run that through opt again with the same limit:

$ opt -S -O2 -o - -non-global-value-max-name-size=4 trunc.ll
opt: trunc.ll:7:3: error: multiple definition of local value named 'reas2'
  %reas2 = shl i32 %reas, 1
  ^

This error comes from a check at the end of LLParser::PerFunctionState::SetInstName. The parser calls Value::setName, which truncates "reas2" to "reas" and then disambiguates it as "reas1". Then it produces the error because Value::getName() = "reas1" isn't the original name "reas2".

Despite such long names having limited practical use, I think we should generally be able to parse our own output. One possibility would be to look for a disambiguation scheme that doesn't make the names longer. But I think that's pretty hard, and it would still produce strange errors when users actually produce long names on their own. The other possibility is that we skip the truncation when called from the LLParser. Off the top of my head, I see three possibilities:

1. Add a boolean parameter to setName(). Not ideal.
2. Create another function like setName() (factoring out common parts) that doesn't truncate, and while we are at it, also doesn't discard local value names. That would make the check for Context.shouldDiscardValueNames() in LLParser::Run() obsolete.
3. Add another field to LLVMContext. We already have the DiscardValueNames property, so why not add a TruncateValueNames? But then we would need to make sure it's set correctly in LLParser::Run() just like the other one.

I slightly prefer variant 2, although 3 is probably more in line with the existing code. Any ideas or opinions about that?

[uabelho]

In D41296#1607583, @aaronpuchert wrote:

In D41296#962215, @hfinkel wrote:

Why don't we just cap all names at some point (and then just start adding numbers as we generally do to break degeneracies). It seems like, otherwise, we'll end up with these kinds of fixes in many places. Fixing this in one common place seems better.

This change has the unfortunate side effect of creating unexpected error messages in the LLParser that have been observed in software using LLVM. Reading the discussion here does not indicate that this effect was intended.

It can even lead to cases where we can't read our own output. To see how this happens, let's simulate a much smaller limit of 4. The normal output of the test case would have been (I've stripped the metadata here):

I think the above problem is similar to what is described in
https://bugs.llvm.org/show_bug.cgi?id=45899

There is a patch trying to address it:
https://reviews.llvm.org/D102707

llvm/trunk/lib/IR/Value.cpp

 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace llvm;
 
+static cl::opt<unsigned> NonGlobalValueMaxNameSize(
+    "non-global-value-max-name-size", cl::Hidden, cl::init(1024),
+    cl::desc("Maximum size for the name of non-global values."));
+
 //===----------------------------------------------------------------------===//
 //                                Value Class
 //===----------------------------------------------------------------------===//
 static inline Type *checkType(Type *Ty) {
   assert(Ty && "Value defined with a null type: Error!");
   return Ty;
 }
 
void Value::setNameImpl(const Twine &NewName) {
   StringRef NameRef = 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;
 
+  // Cap the size of non-GlobalValue names.
+  if (NameRef.size() > NonGlobalValueMaxNameSize && !isa<GlobalValue>(this))
+    NameRef =
+        NameRef.substr(0, std::max(1u, (unsigned)NonGlobalValueMaxNameSize));
+
   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.

llvm/trunk/test/Bitcode/value-with-long-name.ll

+; Check the size of generated variable when no option is set
+; RUN: opt -S %s -O2 -o - | FileCheck -check-prefix=CHECK-LONG %s
+; CHECK-LONG: %{{[a-z]{4}[a-z]+}}
+
+; Then check we correctly cap the size of newly generated non-global values name
+; Force the size to be small so that the check works on release and debug build
+; RUN: opt -S %s -O2 -o - -non-global-value-max-name-size=0 | FileCheck -check-prefix=CHECK-SHORT %s
+; RUN: opt -S %s -O2 -o - -non-global-value-max-name-size=1 | FileCheck -check-prefix=CHECK-SHORT %s
+; CHECK-SHORT-NOT: %{{[a-z][a-z]+}}
+
+define i32 @f(i32 %a, i32 %b) {
+  %c = add i32 %a, %b
+  %d = add i32 %c, %a
+  %e = add i32 %d, %b
+  ret i32 %e
+}
+
+