Add a flag to the LLVMContext to disable name for Value other than GlobalValue

[mehdi_amini]

Summary

This is intended to be a performance flag, on the same level as clang
cc1 option "--disable-free". LLVM will never initialize it by default,
it will be up to the client creating the LLVMContext to request this
behavior. Clang will do it by default in Release build (just like
--disable-free).

"opt" and "llc" can opt-in using -disable-named-value command line
option.

When performing LTO on llvm-tblgen, the initial merging of IR peaks
at 92MB without this patch, and 86MB after this patch,setNameImpl()
drops from 6.5MB to 0.5MB.
The total link time goes from ~29.5s to ~27.8s.

[tejohnson]

What about from llvm-lto, do you want an option there too? Right now it will use the NDEBUG behavior in LTOCodeGenerator's constructor.

Note to self: add similar flag and set on context from gold plugin too when this goes in.

lib/AsmParser/LLParser.cpp

52

Do you mean "Can't read..."?

[mehdi_amini]

Good point, I haven't thought about llvm-lto. Looks like I need to find some way to push this out of the LTOCodeGenerator (I'll wait to see what others think about the direction taken by this patch first).

lib/AsmParser/LLParser.cpp

52

Indeed :-)

[mehdi_amini]

Note: this would make the first template parameter of the IR builder "PreserveName" obsolete.
Some passes such as SROA were doing:

/// \brief Provide a typedef for IRBuilder that drops names in release builds.
#ifndef NDEBUG
typedef llvm::IRBuilder<true, ConstantFolder, IRBuilderPrefixedInserter<true>>
    IRBuilderTy;
#else
typedef llvm::IRBuilder<false, ConstantFolder, IRBuilderPrefixedInserter<false>>
    IRBuilderTy;
#endif
}

I thought about applying this runtime check in the IRBuilder itself, but many passes are not going through the IRBuilder and creates value directly.

[• rafael]

This looks like a good idea, but you should delete the PreserveName
option in a followup commit :-)

[mehdi_amini]

Rename preserveNames() to preserveNonGlobalValueNames()

[chandlerc]

lib/AsmParser/LLParser.cpp

52

FWIW, latest version of patch still has "Can read ..." here. =]

lib/LTO/LTOCodeGenerator.cpp

83–85

It'd be nice to make this be flag-controlled as well instead of NDEBUG controlled. Is that not possible?

tools/llc/llc.cpp

107

"disable-named-values" maybe?

213

No need for the comment, the code seems quite clear.

tools/opt/opt.cpp

200

Same here.

[mehdi_amini]

Update, trying to account for comments (hopefully I didn't forget any)

I propose to name these: discardValueNames() / setDiscardValueNames(), Justin what do you think?

Chandler: I tries to time on clang -O3 on a large .cpp file but it didn't have much impact (setValueName is a very small part in this case apparently)

[chandlerc]

LGTM with the nits below fixed and with whatever name we finally settle on. (We can also tweak the name later if needed). But sync up with Justin before you land it to be sure he's OK.

include/llvm/IR/LLVMContext.h

115

The parameter name seems... confusing. ;] Probably want this to be "Discard" now.

lib/IR/LLVMContextImpl.h

1037–1039

I would make this consistent with the interface (DiscardValueNames).

lib/LTO/LTOCodeGenerator.cpp

68–69

I would probably name the flags to exactly match the context: '-discard-value-names' (or whatever that ends up being).

[mehdi_amini]

Take Chandler's comment into account

[mehdi_amini]

lib/IR/LLVMContextImpl.h

1037–1039

Ohhhh it is event worse than a just name bikeshedding, there is unintended behavior change here! The default would have been to Discard names...
(I didn't notice in the tests because I only ran opt/llc tests with this last change and these will always explicitly set it to false based on the cl::opt, it would have impacted other llvm clients)

[reames]

I don't really care if this lands in it's current form, but I'm somewhat of the belief this is the wrong approach to the problem.

An alternate approach which would seem more natural:

• We introduce an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks.
• We add a utility pass which drops names from instructions and basic blocks.
• Clang optionally schedules the utility pass early in it's pipeline.

[mehdi_amini]

Hey Philip,

Thanks for chiming in.
Can you articulate what you have in mind for "an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks."?
I don't visualize how it would be expressed in practice, unless you intend to audit all the passes and change all the places where prefix/suffix are introduced?

[mehdi_amini]

Update to committed rev

[mehdi_amini]

r263086

[reames]

In D17946#371188, @joker.eph wrote:

Hey Philip,

Thanks for chiming in.
Can you articulate what you have in mind for "an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks."?
I don't visualize how it would be expressed in practice, unless you intend to audit all the passes and change all the places where prefix/suffix are introduced?

Yes. If we have code which unconditionally adds a suffix, it would become if (hasName() ? getName() + "suffix" : ""). This seems entirely reasonable.

[chandlerc]

LGTM with the nits below fixed and with whatever name we finally settle on. (We can also tweak the name later if needed). But sync up with Justin before you land it to be sure he's OK.

In D17946#371474, @reames wrote:

In D17946#371188, @joker.eph wrote:

Hey Philip,

Thanks for chiming in.
Can you articulate what you have in mind for "an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks."?
I don't visualize how it would be expressed in practice, unless you intend to audit all the passes and change all the places where prefix/suffix are introduced?

Yes. If we have code which unconditionally adds a suffix, it would become if (hasName() ? getName() + "suffix" : ""). This seems entirely reasonable.

I totally understand why you want the names -- I agree they're incredibly important for debugging.

I'm surprised you don't find the approach Mehdi is pursuing here strictly superior though. This essentially lets us completely remove the NDEBUG disabling of names. That means you can always pass a flag to Clang and get names in the output for debugging.

I find this specifically nicer than keying on whether the original value had a name because there are a number of passes that produce values without names. Even when they do that, I would very much like to have the name suffixes introduced by subsequent passes when I'm debugging.

Does that make sense?

(I'd rather not go converting all of LLVM and Clang to rely on this approach unless you're comfortable with it...)

[reames]

In D17946#371699, @chandlerc wrote:

LGTM with the nits below fixed and with whatever name we finally settle on. (We can also tweak the name later if needed). But sync up with Justin before you land it to be sure he's OK.

In D17946#371474, @reames wrote:

In D17946#371188, @joker.eph wrote:

Hey Philip,

Thanks for chiming in.
Can you articulate what you have in mind for "an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks."?
I don't visualize how it would be expressed in practice, unless you intend to audit all the passes and change all the places where prefix/suffix are introduced?

Yes. If we have code which unconditionally adds a suffix, it would become if (hasName() ? getName() + "suffix" : ""). This seems entirely reasonable.

I totally understand why you want the names -- I agree they're incredibly important for debugging.

I'm surprised you don't find the approach Mehdi is pursuing here strictly superior though. This essentially lets us completely remove the NDEBUG disabling of names. That means you can always pass a flag to Clang and get names in the output for debugging.

I find this specifically nicer than keying on whether the original value had a name because there are a number of passes that produce values without names. Even when they do that, I would very much like to have the name suffixes introduced by subsequent passes when I'm debugging.

I hadn't thought about this aspect. Good point.

Does that make sense?

(I'd rather not go converting all of LLVM and Clang to rely on this approach unless you're comfortable with it...)

I don't have a strong opinion here. I was pointing out what seemed like a natural approach to me, but I'm also more than comfortable deferring to you and others who have thought about this more. Provided that the default is to have names even in release mode (default for LLVM, not necessarily clang), I'm comfortable with whatever direction you want to take this.

[chandlerc]

In D17946#372751, @reames wrote:

In D17946#371699, @chandlerc wrote:

LGTM with the nits below fixed and with whatever name we finally settle on. (We can also tweak the name later if needed). But sync up with Justin before you land it to be sure he's OK.

In D17946#371474, @reames wrote:

In D17946#371188, @joker.eph wrote:

Hey Philip,

Thanks for chiming in.
Can you articulate what you have in mind for "an invariant for the optimizer wherein we only introduce named instructions if the instruction being replaced was named. Same with basic blocks."?
I don't visualize how it would be expressed in practice, unless you intend to audit all the passes and change all the places where prefix/suffix are introduced?

Yes. If we have code which unconditionally adds a suffix, it would become if (hasName() ? getName() + "suffix" : ""). This seems entirely reasonable.

I totally understand why you want the names -- I agree they're incredibly important for debugging.

I'm surprised you don't find the approach Mehdi is pursuing here strictly superior though. This essentially lets us completely remove the NDEBUG disabling of names. That means you can always pass a flag to Clang and get names in the output for debugging.

I find this specifically nicer than keying on whether the original value had a name because there are a number of passes that produce values without names. Even when they do that, I would very much like to have the name suffixes introduced by subsequent passes when I'm debugging.

I hadn't thought about this aspect. Good point.

Does that make sense?

(I'd rather not go converting all of LLVM and Clang to rely on this approach unless you're comfortable with it...)

I don't have a strong opinion here. I was pointing out what seemed like a natural approach to me, but I'm also more than comfortable deferring to you and others who have thought about this more. Provided that the default is to have names even in release mode (default for LLVM, not necessarily clang), I'm comfortable with whatever direction you want to take this.

Awesome, and thanks for following up.

I think this will end up with essentially the best of all worlds in that at all places where we can have extra information we will, and when we don't need it, we can turn off the cost of preserving it.

include/llvm/IR/LLVMContext.h

public:
   void setGC(const Function &Fn, std::string GCName);
 
   /// Return the GC for a function
   const std::string &getGC(const Function &Fn);
 
   /// Remove the GC for a function
   void deleteGC(const Function &Fn);
 
+  /// Return true if the Context runtime configuration is set to discard all
+  /// value names. When true, only GlobalValue names will be available in the
+  /// IR.
+  bool discardValueNames();
+
+  /// Set the Context runtime configuration to discard all value name (but
+  /// GlobalValue). Clients can use this flag to save memory and runtime,
+  /// especially in release mode.
+  void setDiscardValueNames(bool Discard);
 
   typedef void (*InlineAsmDiagHandlerTy)(const SMDiagnostic&, void *Context,
                                          unsigned LocCookie);
 
   /// Defines the type of a diagnostic handler.
   /// \see LLVMContext::setDiagnosticHandler.
   /// \see LLVMContext::diagnose.
   typedef void (*DiagnosticHandlerTy)(const DiagnosticInfo &DI, void *Context);

lib/AsmParser/LLParser.cpp

static std::string getTypeString(Type *T) {
   return Tmp.str();
 }
 
 /// Run: module ::= toplevelentity*
 bool LLParser::Run() {
   // Prime the lexer.
   Lex.Lex();
 
+  if (Context.discardValueNames())
+    return Error(
+        Lex.getLoc(),
+        "Can't read textual IR with a Context that discards named Values");
+
   return ParseTopLevelEntities() ||
          ValidateEndOfModule();
 }
 
 bool LLParser::parseStandaloneConstantValue(Constant *&C,
                                             const SlotMapping *Slots) {
   restoreParsingState(Slots);
   Lex.Lex();

lib/IR/LLVMContext.cpp

void LLVMContext::setGC(const Function &Fn, std::string GCName) {
   It->second = std::move(GCName);
 }
 const std::string &LLVMContext::getGC(const Function &Fn) {
   return pImpl->GCNames[&Fn];
 }
 void LLVMContext::deleteGC(const Function &Fn) {
   pImpl->GCNames.erase(&Fn);
 }
+
+bool LLVMContext::discardValueNames() { return pImpl->DiscardValueNames; }
+
+void LLVMContext::setDiscardValueNames(bool Discard) {
+  pImpl->DiscardValueNames = Discard;
+}

lib/IR/LLVMContextImpl.h

#include "llvm/IR/Metadata.def"
 
   /// Maintain the GC name for each function.
   ///
   /// This saves allocating an additional word in Function for programs which
   /// do not use GC (i.e., most programs) at the cost of increased overhead for
   /// clients which do use GC.
   DenseMap<const Function*, std::string> GCNames;
 
+  /// Flag to indicate if Value (other than GlobalValue) retains their name or
+  /// not.
+  bool DiscardValueNames = false;
+
   LLVMContextImpl(LLVMContext &C);
   ~LLVMContextImpl();
 
   /// Destroy the ConstantArrays if they are not used.
   void dropTriviallyDeadConstantArrays();
 };
 
 }
 
 #endif

lib/IR/Value.cpp

StringRef Value::getName() const {
   // some clients want to call .data() on the result and expect it to be null
   // terminated.
   if (!hasName())
     return StringRef("", 0);
   return getValueName()->getKey();
 }
 
 void Value::setNameImpl(const Twine &NewName) {
+  // Fast-path: LLVMContext can be set to strip out non-GlobalValue names
+  if (getContext().discardValueNames() && !isa<GlobalValue>(this))
+    return;
+
   // Fast path for common IRBuilder case of setName("") when there is no name.
   if (NewName.isTriviallyEmpty() && !hasName())
     return;
 
   SmallString<256> NameData;
   StringRef NameRef = NewName.toStringRef(NameData);
   assert(NameRef.find_first_of(0) == StringRef::npos &&
          "Null bytes are not allowed in names");

lib/LTO/LTOCodeGenerator.cpp

 const char* LTOCodeGenerator::getVersionString() {
 #ifdef LLVM_VERSION_INFO
   return PACKAGE_NAME " version " PACKAGE_VERSION ", " LLVM_VERSION_INFO;
 #else
   return PACKAGE_NAME " version " PACKAGE_VERSION;
 #endif
 }
 
+namespace llvm {
+cl::opt<bool> LTODiscardValueNames(
+    "discard-value-names",
+    cl::desc("Strip names from Value (other than GlobalValue)."),
+#ifdef NDEBUG
+    cl::init(true),
+#else
+    cl::init(false),
+#endif
+    cl::Hidden);
+}
+
 LTOCodeGenerator::LTOCodeGenerator(LLVMContext &Context)
     : Context(Context), MergedModule(new Module("ld-temp.o", Context)),
       TheLinker(new Linker(*MergedModule)) {
+  Context.setDiscardValueNames(LTODiscardValueNames);
   initializeLTOPasses();
 }
 
 LTOCodeGenerator::~LTOCodeGenerator() {}
 
 // Initialize LTO passes. Please keep this function in sync with
 // PassManagerBuilder::populateLTOPassManager(), and make sure all LTO
 // passes are initialized.
 void LTOCodeGenerator::initializeLTOPasses() {
   PassRegistry &R = *PassRegistry::getPassRegistry();

lib/LTO/ThinLTOCodeGenerator.cpp

 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/FunctionImport.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/ObjCARC.h"
 #include "llvm/Transforms/Utils/FunctionImportUtils.h"
 
 using namespace llvm;
 
+namespace llvm {
+// Flags -discard-value-names, defined in LTOCodeGenerator.cpp
+extern cl::opt<bool> LTODiscardValueNames;
+}
+
 namespace {
 
 static cl::opt<int> ThreadCount("threads",
                                 cl::init(std::thread::hardware_concurrency()));
 
 static void diagnosticHandler(const DiagnosticInfo &DI) {
   DiagnosticPrinterRawOStream DP(errs());
   DI.print(DP);
void ThinLTOCodeGenerator::run() {
 
   // Parallel optimizer + codegen
   {
     ThreadPool Pool(ThreadCount);
     int count = 0;
     for (auto &ModuleBuffer : Modules) {
       Pool.async([&](int count) {
         LLVMContext Context;
+        Context.setDiscardValueNames(LTODiscardValueNames);
 
         // Parse module now
         auto TheModule = loadModuleFromBuffer(ModuleBuffer, Context, false);
 
         // Save temps: original file.
         if (!SaveTempsDir.empty()) {
           saveTempBitcode(*TheModule, SaveTempsDir, count, ".0.original.bc");
         }

test/Feature/strip_names.ll

+; RUN: opt < %s -S | FileCheck %s
+; RUN: opt < %s  | opt -S -discard-value-names | FileCheck --check-prefix=NONAME %s
+
+
+; CHECK: @GlobalValueName
+; CHECK: @foo(i32 %in)
+; CHECK: somelabel:
+; CHECK:  %GV = load i32, i32* @GlobalValueName
+; CHECK:  %add = add i32 %in, %GV
+; CHECK:  ret i32 %add
+
+; NONAME: @GlobalValueName
+; NONAME: @foo(i32)
+; NONAME-NOT: somelabel:
+; NONAME:  %2 = load i32, i32* @GlobalValueName
+; NONAME:  %3 = add i32 %0, %2
+; NONAME:  ret i32 %3
+
+@GlobalValueName = global i32 0
+
+define i32 @foo(i32 %in) {
+somelabel:
+  %GV = load i32, i32* @GlobalValueName
+  %add = add i32 %in, %GV
+  ret i32 %add
+}

tools/llc/llc.cpp

static cl::opt<bool> AsmVerbose("asm-verbose",
                                 cl::init(true));
 
 static cl::opt<bool>
     CompileTwice("compile-twice", cl::Hidden,
                  cl::desc("Run everything twice, re-using the same pass "
                           "manager and verify the result is the same."),
                  cl::init(false));
 
+static cl::opt<bool> DiscardValueNames(
+    "discard-value-names",
+    cl::desc("Discard names from Value (other than GlobalValue)."),
+    cl::init(false), cl::Hidden);
+
 static int compileModule(char **, LLVMContext &);
 
 static std::unique_ptr<tool_output_file>
 GetOutputStream(const char *TargetName, Triple::OSType OS,
                 const char *ProgName) {
   // If we don't yet have an output filename, make one.
   if (OutputFilename.empty()) {
     if (InputFilename == "-")
int main(int argc, char **argv) {
   initializeLowerIntrinsicsPass(*Registry);
   initializeUnreachableBlockElimPass(*Registry);
 
   // Register the target printer for --version.
   cl::AddExtraVersionPrinter(TargetRegistry::printRegisteredTargetsForVersion);
 
   cl::ParseCommandLineOptions(argc, argv, "llvm system compiler\n");
 
+  Context.setDiscardValueNames(DiscardValueNames);
+
   // Compile the module TimeCompilations times to give better compile time
   // metrics.
   for (unsigned I = TimeCompilations; I; --I)
     if (int RetVal = compileModule(argv, Context))
       return RetVal;
   return 0;
 }
 

tools/opt/opt.cpp

static cl::opt<bool> PreserveAssemblyUseListOrder(
     cl::desc("Preserve use-list order when writing LLVM assembly."),
     cl::init(false), cl::Hidden);
 
 static cl::opt<bool>
     RunTwice("run-twice",
              cl::desc("Run all passes twice, re-using the same pass manager."),
              cl::init(false), cl::Hidden);
 
+static cl::opt<bool> DiscardValueNames(
+    "discard-value-names",
+    cl::desc("Discard names from Value (other than GlobalValue)."),
+    cl::init(false), cl::Hidden);
+
 static inline void addPass(legacy::PassManagerBase &PM, Pass *P) {
   // Add the pass to the pass manager...
   PM.add(P);
 
   // If we are verifying all of the intermediate steps, add the verifier...
   if (VerifyEach)
     PM.add(createVerifierPass());
 }
#endif
 
   if (AnalyzeOnly && NoOutput) {
     errs() << argv[0] << ": analyze mode conflicts with no-output mode.\n";
     return 1;
   }
 
   SMDiagnostic Err;
 
+  Context.setDiscardValueNames(DiscardValueNames);
+
   // Load the input module...
   std::unique_ptr<Module> M = parseIRFile(InputFilename, Err, Context);
 
   if (!M) {
     Err.print(argv[0], errs());
     return 1;
   }