Page MenuHomePhabricator

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
1 ↗(On Diff #127145)

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 ↗(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 added inline comments.Dec 21 2017, 9:42 PM
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?

@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
Closed by commit rL321886: Limit size of non-GlobalValue name (authored by serge_sans_paille, committed by ). · Explain WhyJan 5 2018, 11:42 AM
This revision was automatically updated to reflect the committed changes.