This is an archive of the discontinued LLVM Phabricator instance.

Limit size of non-GlobalValue name
ClosedPublic

Authored by serge-sans-paille on Dec 15 2017, 8:56 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

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 added a subscriber: eddyb.Dec 15 2017, 11:27 AM

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.

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
2

this etst -> This test

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.

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.

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

serge-sans-paille retitled this revision from Limit size of SROA - generated register names to Limit size of non-GlobalValue name.
serge-sans-paille edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Dec 21 2017, 1:39 PM
lib/IR/Value.cpp
254

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 added inline comments.Dec 21 2017, 9:42 PM
lib/IR/Value.cpp
43

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

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

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?

@chandlerc updated test case and size limit

@chandlerc does this look good to you know?

hfinkel added inline comments.Jan 5 2018, 7:35 AM
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).

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.

serge-sans-paille edited the summary of this revision. (Show Details)
hfinkel accepted this revision.Jan 5 2018, 8:02 AM

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

This revision is now accepted and ready to land.Jan 5 2018, 8:02 AM
This revision was automatically updated to reflect the committed changes.

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?

Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 5:38 PM

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