[PATCH] D14265: DI: Reverse direction of subprogram -> function edge.

Duncan P. N. Exon Smith via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 2 16:42:21 PST 2015 

> On 2015-Nov-02, at 16:27, Peter Collingbourne <peter at pcc.me.uk> wrote:
> 
> pcc created this revision.
> pcc added a reviewer: dexonsmith.
> pcc added a subscriber: llvm-commits.
> Herald added subscribers: dsanders, jyknight, axw.
> 
> Previously, subprograms contained a metadata reference to the function they
> described. Because most clients need to get or set a subprogram for a given
> function rather than the other way around, this created unneeded inefficiency.
> 
> For example, many passes needed to call the function llvm::makeSubprogramMap()
> to build a mapping from functions to subprograms, and the IR linker needed to
> fix up function references in a way that caused quadratic complexity in the IR
> linking phase of LTO.
> 
> This change reverses the direction of the edge by storing the subprogram as
> function-level metadata and removing DISubprogram's function field.
> 
> Since this is an IR change, a bitcode upgrade has been provided.
> 
> Fixes PR23367.
> 
> http://reviews.llvm.org/D14265

This is great, thanks for working on it.

I haven't had a chance to review it in detail yet, I've just looked at
the bitcode part so far.

> Index: lib/Bitcode/Writer/BitcodeWriter.cpp
> ===================================================================
> --- lib/Bitcode/Writer/BitcodeWriter.cpp
> +++ lib/Bitcode/Writer/BitcodeWriter.cpp
> @@ -1024,7 +1024,7 @@
>    Record.push_back(N->getVirtualIndex());
>    Record.push_back(N->getFlags());
>    Record.push_back(N->isOptimized());
> -  Record.push_back(VE.getMetadataOrNullID(N->getRawFunction()));
> +  Record.push_back(0);

This bloats the bitcode files.  Why not remove the field?

>    Record.push_back(VE.getMetadataOrNullID(N->getTemplateParams().get()));
>    Record.push_back(VE.getMetadataOrNullID(N->getDeclaration()));
>    Record.push_back(VE.getMetadataOrNullID(N->getVariables().get()));
> Index: lib/Bitcode/Reader/BitcodeReader.cpp
> ===================================================================
> --- lib/Bitcode/Reader/BitcodeReader.cpp
> +++ lib/Bitcode/Reader/BitcodeReader.cpp
> @@ -2182,17 +2182,23 @@
>        if (Record.size() != 19)
>          return error("Invalid record");
>  
> -      MDValueList.assignValue(
> -          GET_OR_DISTINCT(
> -              DISubprogram,
> -              Record[0] || Record[8], // All definitions should be distinct.
> -              (Context, getMDOrNull(Record[1]), getMDString(Record[2]),
> -               getMDString(Record[3]), getMDOrNull(Record[4]), Record[5],
> -               getMDOrNull(Record[6]), Record[7], Record[8], Record[9],
> -               getMDOrNull(Record[10]), Record[11], Record[12], Record[13],
> -               Record[14], getMDOrNull(Record[15]), getMDOrNull(Record[16]),
> -               getMDOrNull(Record[17]), getMDOrNull(Record[18]))),
> -          NextMDValueNo++);
> +      DISubprogram *SP = GET_OR_DISTINCT(
> +          DISubprogram,
> +          Record[0] || Record[8], // All definitions should be distinct.
> +          (Context, getMDOrNull(Record[1]), getMDString(Record[2]),
> +           getMDString(Record[3]), getMDOrNull(Record[4]), Record[5],
> +           getMDOrNull(Record[6]), Record[7], Record[8], Record[9],
> +           getMDOrNull(Record[10]), Record[11], Record[12], Record[13],
> +           Record[14], getMDOrNull(Record[16]), getMDOrNull(Record[17]),
> +           getMDOrNull(Record[18])));
> +      MDValueList.assignValue(SP, NextMDValueNo++);
> +
> +      // Upgrade sp->function mapping to function->sp mapping.
> +      if (Record[15]) {

After cleaning out the old field, this will be something like:
--
if (Record.size() == 19) {
--

> +        if (auto *CMD = dyn_cast<ConstantAsMetadata>(getMDOrNull(Record[15])))
> +          if (auto *F = dyn_cast<Function>(CMD->getValue()))
> +            F->setSubprogram(SP);

This call is only valid if `F` is a definition.  Otherwise it will
assert.

IIRC, metadata typically gets parsed *before* function bodies, so I
think this could even rarely be valid?  I didn't look at tests... did
you have an upgrade test?

Anyway, what I'd recommend instead is:
--
auto *F = dyn_cast<Function>(CMD->getValue());
if (F && !F->isDeclaration()) {
  F->setSubprogram(SP);
} else {
  FunctionsWithSPs[CMD->getValue()] = SP;
}
--
Then adding a check into `FunctionsWithSPs` when parsing function
bodies.  It should probably be a `SmallDenseMap` of some sort.

> +      }
>        break;
>      }
>      case bitc::METADATA_LEXICAL_BLOCK: {

> 
> Files:
>  bindings/go/llvm/DIBuilderBindings.cpp
>  bindings/go/llvm/DIBuilderBindings.h
>  bindings/go/llvm/IRBindings.cpp
>  bindings/go/llvm/IRBindings.h
>  bindings/go/llvm/dibuilder.go
>  bindings/go/llvm/ir.go
>  include/llvm/IR/DIBuilder.h
>  include/llvm/IR/DebugInfo.h
>  include/llvm/IR/DebugInfoMetadata.h
>  lib/AsmParser/LLParser.cpp
>  lib/Bitcode/Reader/BitcodeReader.cpp
>  lib/Bitcode/Writer/BitcodeWriter.cpp
>  lib/CodeGen/AsmPrinter/DwarfDebug.cpp
>  lib/CodeGen/LiveDebugVariables.cpp
>  lib/IR/AsmWriter.cpp
>  lib/IR/DIBuilder.cpp
>  lib/IR/DebugInfo.cpp
>  lib/IR/DebugInfoMetadata.cpp
>  lib/IR/LLVMContextImpl.h
>  lib/IR/Verifier.cpp
>  lib/Linker/LinkModules.cpp
>  lib/Transforms/IPO/ArgumentPromotion.cpp
>  lib/Transforms/IPO/DeadArgumentElimination.cpp
>  lib/Transforms/IPO/StripSymbols.cpp
>  lib/Transforms/Instrumentation/DataFlowSanitizer.cpp
>  lib/Transforms/Instrumentation/GCOVProfiling.cpp
>  lib/Transforms/Utils/CloneFunction.cpp
>  test/Assembler/2010-02-05-FunctionLocalMetadataBecomesNull.ll
>  test/Assembler/disubprogram.ll
>  test/Assembler/drop-debug-info.ll
>  test/Bitcode/upgrade-subprogram.ll
>  test/Bitcode/upgrade-subprogram.ll.bc
>  test/CodeGen/AArch64/aarch64-2014-08-11-MachineCombinerCrash.ll
>  test/CodeGen/AMDGPU/llvm.dbg.value.ll
>  test/CodeGen/ARM/2010-08-04-StackVariable.ll
>  test/CodeGen/ARM/2011-01-19-MergedGlobalDbg.ll
>  test/CodeGen/ARM/2011-08-02-MergedGlobalDbg.ll
>  test/CodeGen/ARM/coalesce-dbgvalue.ll
>  test/CodeGen/ARM/debug-frame-vararg.ll
>  test/CodeGen/ARM/debug-frame.ll
>  test/CodeGen/ARM/debug-info-arg.ll
>  test/CodeGen/ARM/debug-info-blocks.ll
>  test/CodeGen/ARM/debug-info-branch-folding.ll
>  test/CodeGen/ARM/debug-info-d16-reg.ll
>  test/CodeGen/ARM/debug-info-no-frame.ll
>  test/CodeGen/ARM/debug-info-qreg.ll
>  test/CodeGen/ARM/debug-info-s16-reg.ll
>  test/CodeGen/ARM/debug-info-sreg2.ll
>  test/CodeGen/ARM/debug-segmented-stacks.ll
>  test/CodeGen/ARM/sched-it-debug-nodes.ll
>  test/CodeGen/ARM/vfp-regs-dwarf.ll
>  test/CodeGen/Hexagon/cfi-late.ll
>  test/CodeGen/Hexagon/hwloop-dbg.ll
>  test/CodeGen/Inputs/DbgValueOtherTargets.ll
>  test/CodeGen/MIR/X86/expected-metadata-node-after-debug-location.mir
>  test/CodeGen/MIR/X86/expected-metadata-node-after-exclaim.mir
>  test/CodeGen/MIR/X86/instructions-debug-location.mir
>  test/CodeGen/MIR/X86/metadata-operands.mir
>  test/CodeGen/MIR/X86/unknown-metadata-node.mir
>  test/CodeGen/PowerPC/dbg.ll
>  test/CodeGen/PowerPC/pr17168.ll
>  test/CodeGen/PowerPC/pr24546.ll
>  test/CodeGen/PowerPC/unwind-dw2-g.ll
>  test/CodeGen/WinEH/wineh-cloning.ll
>  test/CodeGen/X86/2010-01-18-DbgValue.ll
>  test/CodeGen/X86/2010-05-25-DotDebugLoc.ll
>  test/CodeGen/X86/2010-05-26-DotDebugLoc.ll
>  test/CodeGen/X86/2010-05-28-Crash.ll
>  test/CodeGen/X86/2010-06-01-DeadArg-DbgInfo.ll
>  test/CodeGen/X86/2010-08-04-StackVariable.ll
>  test/CodeGen/X86/2010-09-16-EmptyFilename.ll
>  test/CodeGen/X86/2010-11-02-DbgParameter.ll
>  test/CodeGen/X86/2011-01-24-DbgValue-Before-Use.ll
>  test/CodeGen/X86/2012-11-30-handlemove-dbg.ll
>  test/CodeGen/X86/2012-11-30-misched-dbg.ll
>  test/CodeGen/X86/2012-11-30-regpres-dbg.ll
>  test/CodeGen/X86/MachineSink-DbgValue.ll
>  test/CodeGen/X86/dbg-changes-codegen-branch-folding.ll
>  test/CodeGen/X86/dbg-combine.ll
>  test/CodeGen/X86/debugloc-argsize.ll
>  test/CodeGen/X86/fpstack-debuginstr-kill.ll
>  test/CodeGen/X86/machine-trace-metrics-crash.ll
>  test/CodeGen/X86/misched-code-difference-with-debug.ll
>  test/CodeGen/X86/null-streamer.ll
>  test/CodeGen/X86/stack-protector-dbginfo.ll
>  test/CodeGen/X86/unknown-location.ll
>  test/CodeGen/XCore/dwarf_debug.ll
>  test/DebugInfo/AArch64/cfi-eof-prologue.ll
>  test/DebugInfo/AArch64/coalescing.ll
>  test/DebugInfo/AArch64/constant-dbgloc.ll
>  test/DebugInfo/AArch64/dwarfdump.ll
>  test/DebugInfo/AArch64/frameindices.ll
>  test/DebugInfo/AArch64/struct_by_value.ll
>  test/DebugInfo/ARM/PR16736.ll
>  test/DebugInfo/ARM/cfi-eof-prologue.ll
>  test/DebugInfo/ARM/constant-dbgloc.ll
>  test/DebugInfo/ARM/float-args.ll
>  test/DebugInfo/ARM/header.ll
>  test/DebugInfo/ARM/lowerbdgdeclare_vla.ll
>  test/DebugInfo/ARM/multiple-constant-uses-drops-dbgloc.ll
>  test/DebugInfo/ARM/s-super-register.ll
>  test/DebugInfo/ARM/selectiondag-deadcode.ll
>  test/DebugInfo/ARM/single-constant-use-preserves-dbgloc.ll
>  test/DebugInfo/COFF/asan-module-ctor.ll
>  test/DebugInfo/COFF/asm.ll
>  test/DebugInfo/COFF/cpp-mangling.ll
>  test/DebugInfo/COFF/multifile.ll
>  test/DebugInfo/COFF/multifunction.ll
>  test/DebugInfo/COFF/simple.ll
>  test/DebugInfo/COFF/tail-call-without-lexical-scopes.ll
>  test/DebugInfo/Generic/2009-11-05-DeadGlobalVariable.ll
>  test/DebugInfo/Generic/2009-11-10-CurrentFn.ll
>  test/DebugInfo/Generic/2010-03-24-MemberFn.ll
>  test/DebugInfo/Generic/2010-04-06-NestedFnDbgInfo.ll
>  test/DebugInfo/Generic/2010-04-19-FramePtr.ll
>  test/DebugInfo/Generic/2010-05-10-MultipleCU.ll
>  test/DebugInfo/Generic/2010-06-29-InlinedFnLocalVar.ll
>  test/DebugInfo/Generic/2010-07-19-Crash.ll
>  test/DebugInfo/Generic/2010-10-01-crash.ll
>  test/DebugInfo/Generic/Inputs/gmlt.ll
>  test/DebugInfo/Generic/PR20038.ll
>  test/DebugInfo/Generic/array.ll
>  test/DebugInfo/Generic/block-asan.ll
>  test/DebugInfo/Generic/constant-pointers.ll
>  test/DebugInfo/Generic/constant-sdnodes-have-dbg-location.ll
>  test/DebugInfo/Generic/constantfp-sdnodes-have-dbg-location.ll
>  test/DebugInfo/Generic/cross-cu-inlining.ll
>  test/DebugInfo/Generic/cross-cu-linkonce-distinct.ll
>  test/DebugInfo/Generic/cross-cu-linkonce.ll
>  test/DebugInfo/Generic/cu-range-hole.ll
>  test/DebugInfo/Generic/cu-ranges.ll
>  test/DebugInfo/Generic/dead-argument-order.ll
>  test/DebugInfo/Generic/debug-info-qualifiers.ll
>  test/DebugInfo/Generic/debuginfofinder-multiple-cu.ll
>  test/DebugInfo/Generic/def-line.ll
>  test/DebugInfo/Generic/dwarf-public-names.ll
>  test/DebugInfo/Generic/enum-types.ll
>  test/DebugInfo/Generic/enum.ll
>  test/DebugInfo/Generic/global.ll
>  test/DebugInfo/Generic/gvn.ll
>  test/DebugInfo/Generic/incorrect-variable-debugloc.ll
>  test/DebugInfo/Generic/incorrect-variable-debugloc1.ll
>  test/DebugInfo/Generic/inline-debug-info-multiret.ll
>  test/DebugInfo/Generic/inline-debug-info.ll
>  test/DebugInfo/Generic/inline-no-debug-info.ll
>  test/DebugInfo/Generic/inline-scopes.ll
>  test/DebugInfo/Generic/inlined-arguments.ll
>  test/DebugInfo/Generic/inlined-vars.ll
>  test/DebugInfo/Generic/location-verifier.ll
>  test/DebugInfo/Generic/lto-comp-dir.ll
>  test/DebugInfo/Generic/member-order.ll
>  test/DebugInfo/Generic/missing-abstract-variable.ll
>  test/DebugInfo/Generic/multiline.ll
>  test/DebugInfo/Generic/namespace.ll
>  test/DebugInfo/Generic/namespace_function_definition.ll
>  test/DebugInfo/Generic/namespace_inline_function_definition.ll
>  test/DebugInfo/Generic/piece-verifier.ll
>  test/DebugInfo/Generic/recursive_inlining.ll
>  test/DebugInfo/Generic/restrict.ll
>  test/DebugInfo/Generic/sugared-constants.ll
>  test/DebugInfo/Generic/tu-composite.ll
>  test/DebugInfo/Generic/two-cus-from-same-file.ll
>  test/DebugInfo/Generic/unconditional-branch.ll
>  test/DebugInfo/Generic/varargs.ll
>  test/DebugInfo/Generic/version.ll
>  test/DebugInfo/Inputs/gmlt.ll
>  test/DebugInfo/Inputs/line.ll
>  test/DebugInfo/Mips/InlinedFnLocalVar.ll
>  test/DebugInfo/Mips/delay-slot.ll
>  test/DebugInfo/Mips/fn-call-line.ll
>  test/DebugInfo/Mips/prologue_end.ll
>  test/DebugInfo/Sparc/gnu-window-save.ll
>  test/DebugInfo/SystemZ/variable-loc.ll
>  test/DebugInfo/X86/2010-04-13-PubType.ll
>  test/DebugInfo/X86/2011-09-26-GlobalVarContext.ll
>  test/DebugInfo/X86/2011-12-16-BadStructRef.ll
>  test/DebugInfo/X86/DW_AT_byte_size.ll
>  test/DebugInfo/X86/DW_AT_linkage_name.ll
>  test/DebugInfo/X86/DW_AT_location-reference.ll
>  test/DebugInfo/X86/DW_AT_object_pointer.ll
>  test/DebugInfo/X86/DW_AT_specification.ll
>  test/DebugInfo/X86/DW_AT_stmt_list_sec_offset.ll
>  test/DebugInfo/X86/InlinedFnLocalVar.ll
>  test/DebugInfo/X86/aligned_stack_var.ll
>  test/DebugInfo/X86/arange-and-stub.ll
>  test/DebugInfo/X86/arguments.ll
>  test/DebugInfo/X86/array.ll
>  test/DebugInfo/X86/array2.ll
>  test/DebugInfo/X86/block-capture.ll
>  test/DebugInfo/X86/byvalstruct.ll
>  test/DebugInfo/X86/coff_debug_info_type.ll
>  test/DebugInfo/X86/coff_relative_names.ll
>  test/DebugInfo/X86/concrete_out_of_line.ll
>  test/DebugInfo/X86/constant-aggregate.ll
>  test/DebugInfo/X86/cu-ranges-odr.ll
>  test/DebugInfo/X86/cu-ranges.ll
>  test/DebugInfo/X86/dbg-byval-parameter.ll
>  test/DebugInfo/X86/dbg-const-int.ll
>  test/DebugInfo/X86/dbg-const.ll
>  test/DebugInfo/X86/dbg-declare-arg.ll
>  test/DebugInfo/X86/dbg-declare.ll
>  test/DebugInfo/X86/dbg-file-name.ll
>  test/DebugInfo/X86/dbg-i128-const.ll
>  test/DebugInfo/X86/dbg-merge-loc-entry.ll
>  test/DebugInfo/X86/dbg-prolog-end.ll
>  test/DebugInfo/X86/dbg-subrange.ll
>  test/DebugInfo/X86/dbg-value-const-byref.ll
>  test/DebugInfo/X86/dbg-value-dag-combine.ll
>  test/DebugInfo/X86/dbg-value-inlined-parameter.ll
>  test/DebugInfo/X86/dbg-value-isel.ll
>  test/DebugInfo/X86/dbg-value-location.ll
>  test/DebugInfo/X86/dbg-value-range.ll
>  test/DebugInfo/X86/dbg-value-terminator.ll
>  test/DebugInfo/X86/dbg_value_direct.ll
>  test/DebugInfo/X86/debug-dead-local-var.ll
>  test/DebugInfo/X86/debug-info-access.ll
>  test/DebugInfo/X86/debug-info-block-captured-self.ll
>  test/DebugInfo/X86/debug-info-blocks.ll
>  test/DebugInfo/X86/debug-info-static-member.ll
>  test/DebugInfo/X86/debug-loc-asan.ll
>  test/DebugInfo/X86/debug-loc-empty-entries.ll
>  test/DebugInfo/X86/debug-loc-offset.ll
>  test/DebugInfo/X86/debug-ranges-offset.ll
>  test/DebugInfo/X86/debug_frame.ll
>  test/DebugInfo/X86/decl-derived-member.ll
>  test/DebugInfo/X86/deleted-bit-piece.ll
>  test/DebugInfo/X86/discriminator.ll
>  test/DebugInfo/X86/dw_op_minus.ll
>  test/DebugInfo/X86/dwarf-aranges-no-dwarf-labels.ll
>  test/DebugInfo/X86/dwarf-aranges.ll
>  test/DebugInfo/X86/dwarf-linkage-names.ll
>  test/DebugInfo/X86/dwarf-public-names.ll
>  test/DebugInfo/X86/dwarf-pubnames-split.ll
>  test/DebugInfo/X86/earlydup-crash.ll
>  test/DebugInfo/X86/elf-names.ll
>  test/DebugInfo/X86/empty-and-one-elem-array.ll
>  test/DebugInfo/X86/ending-run.ll
>  test/DebugInfo/X86/fission-inline.ll
>  test/DebugInfo/X86/fission-ranges.ll
>  test/DebugInfo/X86/float_const.ll
>  test/DebugInfo/X86/formal_parameter.ll
>  test/DebugInfo/X86/frame-register.ll
>  test/DebugInfo/X86/generate-odr-hash.ll
>  test/DebugInfo/X86/ghost-sdnode-dbgvalues.ll
>  test/DebugInfo/X86/gnu-public-names.ll
>  test/DebugInfo/X86/header.ll
>  test/DebugInfo/X86/inline-member-function.ll
>  test/DebugInfo/X86/inline-seldag-test.ll
>  test/DebugInfo/X86/inlined-formal-parameter.ll
>  test/DebugInfo/X86/inlined-indirect-value.ll
>  (144 more files...)
> 
> <D14265.39002.patch>

More information about the llvm-commits mailing list