Always show template parameters in IR type names

[sepavloff]

Summary

If a module contains opaque type and another one contains definition
of equivalent type in sense of C++, llvm-link merges these types.
In this procedure it can rely only on type name. An issue arises if
the type is a class template specialization. See the example.

File 1

template<typename T> struct ABC;
ABC<int> *var_1;

File 2

template<typename T> struct ABC {
  T *f;
};
extern ABC<int> var_1;
ABC<int*> var_2;

llvm-link produces module:

%struct.ABC = type { i32** }
@var_1 = global %struct.ABC* null, align 8
@var_2 = global %struct.ABC zeroinitializer, align 8

Incomplete type ABC<int> from the first module becomes ABC<int*>. It
happens because structure types obtained from template specialization
share the same type name and differ from each other only by version
suffixes, in the example the types are named as ABC and ABC.0.
llvm-link cannot correctly merge these types.

This change enables template arguments in class template specializations.
Clang already prints them in class context, now they will appear in the
class name itself.

[rsmith]

What actual problem is this solving? These IR type names exist only for the convenience of humans reading the IR, and making them long (potentially extremely long) by including template arguments seems likely to hinder that more than it helps.

[sepavloff]

In D40567#940025, @rsmith wrote:

What actual problem is this solving? These IR type names exist only for the convenience of humans reading the IR, and making them long (potentially extremely long) by including template arguments seems likely to hinder that more than it helps.

There is a case when IR type names are essential. If llvm-link sees opaque type in one TU, it tries to merge it with its definition in another TU. The type name is used to identify proper definition. All template specializations share the same name, so llvm-link has to choose random type, which results in incorrect IR. Such IR prevents from some whole-program analyses. This problem cannot be solved by llvm-link, as some information is already lost.

Possible IR bloating due to long type names is indeed an issue. D40508 tries to solve it by replacing long type name or part of it by SHA1 hash. Type names become shorter and still are usable for type identification across different TUs. Template arguments are added to the type names only if compilation produces IR (including embedded IR in LTO), machine code generation is not affected.

If adding template arguments in all cases is not appropriate, probably it makes sense to have an option, something like '--precise-ir', that would turn this generation on? It could be used in the cases when accurate type information in IR is required.

[sepavloff]

Consider a bit more complicated example.

File common.h

#ifndef COMMON_H
#define COMMON_H
struct Alarm { };
struct Info { };
template<typename T> struct Handler;
#endif

File use-1.cpp

#include "common.h"
Handler<Info> *info;
void set(Handler<Info> *e) { info = e; }

File use-2.cpp

#include "common.h"
Handler<Alarm> *alarm;
void set(Handler<Alarm> *e) { alarm = e; }

File defs.h

#ifndef DEFS_H
#define DEFS_H
template<> struct Handler<Info> { int val; };
template<> struct Handler<Alarm> { char val; };
void set(Handler<Info> *);
void set(Handler<Alarm> *);
#endif

File main.cpp

#include "common.h"
#include "defs.h"
void init() {
  Handler<Info> i;
  set(&i);
  Handler<Alarm> a;
  set(&a);
}

Process these files with commands:

clang -c -emit-llvm use-1.cpp -o use-1.cpp.bc
clang -c -emit-llvm use-2.cpp -o use-2.cpp.bc
clang -c -emit-llvm main.cpp -o main.cpp.bc
llvm-link use-1.cpp.bc use-2.cpp.bc main.cpp.bc -S -o test.ll

The resulting IR file has deficiencies:

• The types of the global variables info and alarm are the same:

%struct.Event = type { i32 }
%struct.Event.0 = type { i8 }
@info = global %struct.Event* null, align 8
@alarm = global %struct.Event* null, align 8

• The signatures of the set functions are the same:

define void @_Z4fireP5EventI4InfoE(%struct.Event* %e) #0 {
…
}

define void @_Z4fireP5EventI5AlarmE(%struct.Event* %e) #0 {
…
}

• The call to set(Handler<Alarm> *) is augmented by a bitcast to 'fix' its type:

define void @_Z4fireP5EventI5AlarmE(%struct.Event* %e) #0 {
…
call void bitcast (void (%struct.Event*)* @_Z4fireP5EventI5AlarmE to void (%struct.Event.0*)*)(%struct.Event.0* %a)
…
}

The IR created by llvm-link is distorted. Although code generation (in LTO compilation) might be unaffected by this distortions, other applications of IR linking suffer from it. It does not allow to implement some checks, validation techniques and optimizations.

[sepavloff]

Updated patch

• old type names are generated always if --ir-type-names is not specified,
• added new value of --ir-type-names, none, to suppress type names,
• value of --ir-type-names is stored in module properties.

[rsmith]

In D40567#943747, @sepavloff wrote:

Although code generation (in LTO compilation) might be unaffected by this distortions, other applications of IR linking suffer from it. It does not allow to implement some checks, validation techniques and optimizations.

Any system trying to use IR type names to deduce information about source-level types is simply wrong. We simply don't provide the sort of guarantees you seem to be looking for here. We don't even guarantee to consistently use the same IR type for the same source type within a single translation unit. IR type names exist only for the benefit of humans reading the IR.

[sepavloff]

The feedback here, in D40508 and in the mail list (http://lists.llvm.org/pipermail/llvm-dev/2017-December/119585.html) demonstrates that this is a wrong direction.
Part of this patch is used in D43805, which implements in some sense an opposite approach, - using nameless llvm types.

include/clang/Driver/Options.td

def iframeworkwithsysroot : JoinedOrSeparate<["-"], "iframeworkwithsysroot">,
   MetaVarName<"<directory>">, Flags<[CC1Option]>;
 def imacros : JoinedOrSeparate<["-", "--"], "imacros">, Group<clang_i_Group>, Flags<[CC1Option]>,
   HelpText<"Include macros from file before parsing">, MetaVarName<"<file>">;
 def image__base : Separate<["-"], "image_base">;
 def include_ : JoinedOrSeparate<["-", "--"], "include">, Group<clang_i_Group>, EnumName<"include">,
     MetaVarName<"<file>">, HelpText<"Include file before parsing">, Flags<[CC1Option]>;
 def include_pch : Separate<["-"], "include-pch">, Group<clang_i_Group>, Flags<[CC1Option]>,
   HelpText<"Include precompiled header file">, MetaVarName<"<file>">;
+def ir_type_names_EQ : Joined<["--"], "ir-type-names=">,
+  Flags<[CC1Option, HelpHidden]>,
+  HelpText<"Encoding of IR type names (option: none, terse, full)">,
+  Values<"none,terse,full">;
 def relocatable_pch : Flag<["-", "--"], "relocatable-pch">, Flags<[CC1Option]>,
   HelpText<"Whether to build a relocatable precompiled header">;
 def verify_pch : Flag<["-"], "verify-pch">, Group<Action_Group>, Flags<[CC1Option]>,
   HelpText<"Load and verify that a pre-compiled header file is not stale">;
 def init : Separate<["-"], "init">;
 def install__name : Separate<["-"], "install_name">;
 def iprefix : JoinedOrSeparate<["-"], "iprefix">, Group<clang_i_Group>, Flags<[CC1Option]>,
   HelpText<"Set the -iwithprefix/-iwithprefixbefore prefix">, MetaVarName<"<dir>">;

include/clang/Frontend/CodeGenOptions.h

public:
 
   enum EmbedBitcodeKind {
     Embed_Off,      // No embedded bitcode.
     Embed_All,      // Embed both bitcode and commandline in the output.
     Embed_Bitcode,  // Embed just the bitcode in the output.
     Embed_Marker    // Embed a marker as a placeholder for bitcode.
   };
 
+  /// Describes what kind of names should be specified for LLVM types.
+  ///
+  /// For example, LLVM structure types corresponding to template class
+  /// specializations may be assigned names with template arguments (as
+  /// "struct.Foo<int>") or name with only version suffix (as "Foo.123").
+  ///
+  enum class IRNameKind {
+    Unspecified,    ///< No special option was passed.
+    None,           ///< No type names.
+    Terse,          ///< Only minimal names.
+    Full,           ///< Full names (with template parameters).
+  };
+
   /// The code model to use (-mcmodel).
   std::string CodeModel;
 
   /// The filename with path we use for coverage data files. The runtime
   /// allows further manipulation with the GCOV_PREFIX and GCOV_PREFIX_STRIP
   /// environment variables.
   std::string CoverageDataFile;
 

include/clang/Frontend/CodeGenOptions.def

CODEGENOPT(EmitDeclMetadata  , 1, 0) ///< Emit special metadata indicating what
                                      ///< Only useful when running CodeGen as a
                                      ///< subroutine.
 CODEGENOPT(EmitGcovArcs      , 1, 0) ///< Emit coverage data files, aka. GCDA.
 CODEGENOPT(EmitGcovNotes     , 1, 0) ///< Emit coverage "notes" files, aka GCNO.
 CODEGENOPT(EmitOpenCLArgMetadata , 1, 0) ///< Emit OpenCL kernel arg metadata.
 CODEGENOPT(EmulatedTLS       , 1, 0) ///< Set when -femulated-tls is enabled.
 /// \brief Embed Bitcode mode (off/all/bitcode/marker).
 ENUM_CODEGENOPT(EmbedBitcode, EmbedBitcodeKind, 2, Embed_Off)
+
+/// What kind of names should be specified for LLVM IR types.
+/// \see CodeGenOptions::IRNameKind.
+ENUM_CODEGENOPT(IRTypeNames, IRNameKind, 4, IRNameKind::Unspecified)
+
 CODEGENOPT(ForbidGuardVariables , 1, 0) ///< Issue errors if C++ guard variables
                                         ///< are required.
 CODEGENOPT(FunctionSections  , 1, 0) ///< Set when -ffunction-sections is enabled.
 CODEGENOPT(InstrumentFunctions , 1, 0) ///< Set when -finstrument-functions is
                                        ///< enabled.
 CODEGENOPT(InstrumentFunctionsAfterInlining , 1, 0) ///< Set when
                           ///< -finstrument-functions-after-inlining is enabled.
 CODEGENOPT(InstrumentFunctionEntryBare , 1, 0) ///< Set when

lib/CodeGen/CodeGenModule.cpp

if (   Arch == llvm::Triple::arm
       || Arch == llvm::Triple::armeb
       || Arch == llvm::Triple::thumb
       || Arch == llvm::Triple::thumbeb) {
     // The minimum width of an enum in bytes
     uint64_t EnumWidth = Context.getLangOpts().ShortEnums ? 1 : 4;
     getModule().addModuleFlag(llvm::Module::Error, "min_enum_size", EnumWidth);
   }
 
+  // Opaque type resolution made by IR linker relies on IR type names.
+  if (CodeGenOpts.getIRTypeNames() != CodeGenOptions::IRNameKind::Unspecified)
+    getModule().addModuleFlag(llvm::Module::Error, "type_names",
+                           static_cast<uint32_t>(CodeGenOpts.getIRTypeNames()));
+
   if (CodeGenOpts.SanitizeCfiCrossDso) {
     // Indicate that we want cross-DSO control flow integrity checks.
     getModule().addModuleFlag(llvm::Module::Override, "Cross-DSO CFI", 1);
   }
 
   if (LangOpts.CUDAIsDevice && getTriple().isNVPTX()) {
     // Indicate whether __nvvm_reflect should be configured to flush denormal
     // floating point values to 0.  (This corresponds to its "__CUDA_FTZ"

lib/CodeGen/CodeGenTypes.cpp

 
 const CodeGenOptions &CodeGenTypes::getCodeGenOpts() const {
   return CGM.getCodeGenOpts();
 }
 
 void CodeGenTypes::addRecordTypeName(const RecordDecl *RD,
                                      llvm::StructType *Ty,
                                      StringRef suffix) {
+  if (getCodeGenOpts().getIRTypeNames() == CodeGenOptions::IRNameKind::None)
+    return;
+
   SmallString<256> TypeName;
   llvm::raw_svector_ostream OS(TypeName);
   OS << RD->getKindName() << '.';
-  
   // Name the codegen type after the typedef name
   // if there is no tag type name available
   if (RD->getIdentifier()) {
     // FIXME: We should not have to check for a null decl context here.
     // Right now we do it because the implicit Obj-C decls don't have one.
     if (RD->getDeclContext())
       RD->printQualifiedName(OS);
     else
       RD->printName(OS);
+    if (getCodeGenOpts().getIRTypeNames() == CodeGenOptions::IRNameKind::Full)
+      if (auto *Spec = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
+        const TemplateArgumentList &TemplateArgs = Spec->getTemplateArgs();
+        printTemplateArgumentList(OS, TemplateArgs.asArray(),
+                                  RD->getASTContext().getPrintingPolicy());
+      }
   } else if (const TypedefNameDecl *TDD = RD->getTypedefNameForAnonDecl()) {
     // FIXME: We should not have to check for a null decl context here.
     // Right now we do it because the implicit Obj-C decls don't have one.
     if (TDD->getDeclContext())
       TDD->printQualifiedName(OS);
     else
       TDD->printName(OS);
   } else

lib/Driver/ToolChains/Clang.cpp

if (C.getDriver().embedBitcodeInObject() && !C.getDriver().isUsingLTO() &&
     // Add flags implied by -fembed-bitcode.
     Args.AddLastArg(CmdArgs, options::OPT_fembed_bitcode_EQ);
     // Disable all llvm IR level optimizations.
     CmdArgs.push_back("-disable-llvm-passes");
   }
   if (C.getDriver().embedBitcodeMarkerOnly() && !C.getDriver().isUsingLTO())
     CmdArgs.push_back("-fembed-bitcode=marker");
 
+  Args.AddLastArg(CmdArgs, options::OPT_ir_type_names_EQ);
+
   // We normally speed up the clang process a bit by skipping destructors at
   // exit, but when we're generating diagnostics we can rely on some of the
   // cleanup.
   if (!C.isForDiagnostics())
     CmdArgs.push_back("-disable-free");
 
 // Disable the verification pass in -asserts builds.
 #ifdef NDEBUG

lib/Frontend/CompilerInvocation.cpp

for (const auto &A : Args) {
         StringRef ArgStr(arg);
         Opts.CmdArgs.insert(Opts.CmdArgs.end(), ArgStr.begin(), ArgStr.end());
         // using \00 to seperate each commandline options.
         Opts.CmdArgs.push_back('\0');
       }
     }
   }
 
+  if (Arg *A = Args.getLastArg(OPT_ir_type_names_EQ)) {
+    StringRef Name = A->getValue();
+    auto Info = llvm::StringSwitch<CodeGenOptions::IRNameKind>(Name)
+        .Case("none", CodeGenOptions::IRNameKind::None)
+        .Case("terse", CodeGenOptions::IRNameKind::Terse)
+        .Case("full", CodeGenOptions::IRNameKind::Full)
+        .Default(CodeGenOptions::IRNameKind::Unspecified);
+    if (Info == CodeGenOptions::IRNameKind::Unspecified) {
+      Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Name;
+      Success = false;
+    } else
+      Opts.setIRTypeNames(Info);
+  }
+
   Opts.PreserveVec3Type = Args.hasArg(OPT_fpreserve_vec3_type);
   Opts.InstrumentFunctions = Args.hasArg(OPT_finstrument_functions);
   Opts.InstrumentFunctionsAfterInlining =
       Args.hasArg(OPT_finstrument_functions_after_inlining);
   Opts.InstrumentFunctionEntryBare =
       Args.hasArg(OPT_finstrument_function_entry_bare);
   Opts.XRayInstrumentFunctions = Args.hasArg(OPT_fxray_instrument);
   Opts.XRayAlwaysEmitCustomEvents =

test/CodeGenCXX/template-types.cpp

+// RUN: %clang_cc1 %s -S -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 %s -S -emit-llvm --ir-type-names=none -o - | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang_cc1 %s -S -emit-llvm --ir-type-names=terse -o - | FileCheck %s --check-prefix=CHECK-TERSE
+// RUN: %clang_cc1 %s -S -emit-llvm --ir-type-names=full -o - | FileCheck %s --check-prefix=CHECK-FULL
+
+struct Empty {};
+template<typename T> struct ABC {
+  T v;
+};
+struct Opaque;
+template<typename T> class OpaqueClass;
+
+ABC<int> var_1;
+ABC<ABC<short> > var_2;
+ABC<Empty> var_3;
+ABC<Opaque> *var_4;
+OpaqueClass<int> *var_5;
+OpaqueClass<Empty> *var_6;
+OpaqueClass<Opaque> *var_7;
+OpaqueClass<OpaqueClass<int> > *var_8;
+
+
+//------ without option '--ir-type-names'
+
+// CHECK: %struct.ABC = type { i32 }
+// CHECK: %struct.ABC.0 = type { %struct.ABC.1 }
+// CHECK: %struct.ABC.1 = type { i16 }
+// CHECK: %struct.ABC.2 = type { %struct.Empty }
+// CHECK: %struct.Empty = type { i8 }
+// CHECK: %class.OpaqueClass = type opaque
+// CHECK: %class.OpaqueClass.4 = type opaque
+// CHECK: %class.OpaqueClass.5 = type opaque
+// CHECK: %class.OpaqueClass.6 = type opaque
+
+// CHECK: @var_1 = global %struct.ABC zeroinitializer
+// CHECK: @var_2 = global %struct.ABC.0 zeroinitializer
+// CHECK: @var_3 = global %struct.ABC.2 zeroinitializer
+// CHECK: @var_4 = global %struct.ABC.3* null
+// CHECK: @var_5 = global %class.OpaqueClass* null
+// CHECK: @var_6 = global %class.OpaqueClass.4* null
+// CHECK: @var_7 = global %class.OpaqueClass.5* null
+// CHECK: @var_8 = global %class.OpaqueClass.6* null
+
+// CHECK-NOT: !{{[0-9]+}} = !{{{.*}} !"type_names"
+
+
+//------ with option '--ir-type-names=none'
+
+// CHECK-NONE: %0 = type { i32 }
+// CHECK-NONE: %1 = type { %2 }
+// CHECK-NONE: %2 = type { i16 }
+// CHECK-NONE: %3 = type { %4 }
+// CHECK-NONE: %4 = type { i8 }
+// CHECK-NONE: %5 = type opaque
+// CHECK-NONE: %6 = type opaque
+// CHECK-NONE: %7 = type opaque
+// CHECK-NONE: %8 = type opaque
+// CHECK-NONE: %9 = type opaque
+
+// CHECK-NONE: @var_1 = global %0 zeroinitializer
+// CHECK-NONE: @var_2 = global %1 zeroinitializer
+// CHECK-NONE: @var_3 = global %3 zeroinitializer
+// CHECK-NONE: @var_4 = global %5* null
+// CHECK-NONE: @var_5 = global %6* null
+// CHECK-NONE: @var_6 = global %7* null
+// CHECK-NONE: @var_7 = global %8* null
+// CHECK-NONE: @var_8 = global %9* null
+
+// CHECK-NONE: !{{[0-9]+}} = !{{{.*}} !"type_names", i32 1}
+
+
+//------ with option '--ir-type-names=terse'
+
+// CHECK-TERSE: %struct.ABC = type { i32 }
+// CHECK-TERSE: %struct.ABC.0 = type { %struct.ABC.1 }
+// CHECK-TERSE: %struct.ABC.1 = type { i16 }
+// CHECK-TERSE: %struct.ABC.2 = type { %struct.Empty }
+// CHECK-TERSE: %struct.Empty = type { i8 }
+// CHECK-TERSE: %class.OpaqueClass = type opaque
+// CHECK-TERSE: %class.OpaqueClass.4 = type opaque
+// CHECK-TERSE: %class.OpaqueClass.5 = type opaque
+// CHECK-TERSE: %class.OpaqueClass.6 = type opaque
+
+// CHECK-TERSE: @var_1 = global %struct.ABC zeroinitializer
+// CHECK-TERSE: @var_2 = global %struct.ABC.0 zeroinitializer
+// CHECK-TERSE: @var_3 = global %struct.ABC.2 zeroinitializer
+// CHECK-TERSE: @var_4 = global %struct.ABC.3* null
+// CHECK-TERSE: @var_5 = global %class.OpaqueClass* null
+// CHECK-TERSE: @var_6 = global %class.OpaqueClass.4* null
+// CHECK-TERSE: @var_7 = global %class.OpaqueClass.5* null
+// CHECK-TERSE: @var_8 = global %class.OpaqueClass.6* null
+
+// CHECK-TERSE: !{{[0-9]+}} = !{{{.*}} !"type_names", i32 2}
+
+
+//------ with option '--ir-type-names=full'
+
+// CHECK-FULL: %"struct.ABC<int>" = type { i32 }
+// CHECK-FULL: %"struct.ABC<ABC<short> >" = type { %"struct.ABC<short>" }
+// CHECK-FULL: %"struct.ABC<short>" = type { i16 }
+// CHECK-FULL: %"struct.ABC<Empty>" = type { %struct.Empty }
+// CHECK-FULL: %struct.Empty = type { i8 }
+// CHECK-FULL: %"struct.ABC<Opaque>" = type opaque
+// CHECK-FULL: %"class.OpaqueClass<int>" = type opaque
+// CHECK-FULL: %"class.OpaqueClass<Empty>" = type opaque
+// CHECK-FULL: %"class.OpaqueClass<Opaque>" = type opaque
+// CHECK-FULL: %"class.OpaqueClass<OpaqueClass<int> >" = type opaque
+
+// CHECK-FULL: @var_1 = global %"struct.ABC<int>" zeroinitializer
+// CHECK-FULL: @var_2 = global %"struct.ABC<ABC<short> >" zeroinitializer
+// CHECK-FULL: @var_3 = global %"struct.ABC<Empty>" zeroinitializer
+// CHECK-FULL: @var_4 = global %"struct.ABC<Opaque>"* null
+// CHECK-FULL: @var_5 = global %"class.OpaqueClass<int>"* null
+// CHECK-FULL: @var_6 = global %"class.OpaqueClass<Empty>"* null
+// CHECK-FULL: @var_7 = global %"class.OpaqueClass<Opaque>"* null
+// CHECK-FULL: @var_8 = global %"class.OpaqueClass<OpaqueClass<int> >"* null
+
+// CHECK-FULL:  !{{[0-9]+}} = !{{{.*}} !"type_names", i32 3}