[PATCH] D41296: Limit size of non-GlobalValue name

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/IR/Value.cpp:43
+static cl::opt<std::size_t> NonGlobalValueMaxNameSize(
+    "non-global-value-max-name-size", cl::Hidden, cl::init(64),
+    cl::desc("Maximum size for the name of non-global values."));
----------------
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.


================
Comment at: lib/IR/Value.cpp:254
+    NameRef = NameRef.substr(0, NonGlobalValueMaxNameSize);
+
   assert(!getType()->isVoidTy() && "Cannot assign a name to void values!");
----------------
mehdi_amini wrote:
> 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.
Yeah, but because this is just tweaking the debug build to scale a bit better, I'm OK with this approach.


================
Comment at: test/Transforms/SROA/register-name-too-long.ll:1
+; This test case used to swap because of too many intermediate steps resulting in utterly long names.
+; RUN: opt < %s -sroa -S
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D41296