Page MenuHomePhabricator

[cloning] Do not duplicate types when cloning functions
ClosedPublic

Authored by GorNishanov on Jul 6 2017, 9:52 PM.

Details

Summary

This is an addon to the change rl304488 cloning fixes. (Originally rl304226 reverted rl304228 and reapplied rl304488 https://reviews.llvm.org/D33655)

rl304488 works great when DILocalVariables that comes from the inlined function has a 'unique-ed' type, but,
in the case when the variable type is distinct we will create a second DILocalVariable in the scope of the original function that was inlined.

Consider cloning of the following function:

define private void @f() !dbg !5 {
  %1 = alloca i32, !dbg !11
  call void @llvm.dbg.declare(metadata i32* %1, metadata !14, metadata !12), !dbg !18
  ret void, !dbg !18
}

!14 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !17) ; came from an inlined function
!15 = distinct !DISubprogram(name: "inlined", linkageName: "inlined", scope: null, file: !6, line: 8, type: !7, isLocal: true, isDefinition: true, scopeLine: 9, isOptimized: false, unit: !0, variables: !16)
!16 = !{!14}
!17 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)

Without this fix, when function 'f' is cloned, we will create another DILocalVariable for "inlined", due to its type being distinct.

define private void @f.1() !dbg !23 {
  %1 = alloca i32, !dbg !26
  call void @llvm.dbg.declare(metadata i32* %1, metadata !28, metadata !12), !dbg !30
  ret void, !dbg !30
}

!14 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !17)
!15 = distinct !DISubprogram(name: "inlined", linkageName: "inlined", scope: null, file: !6, line: 8, type: !7, isLocal: true, isDefinition: true, scopeLine: 9, isOptimized: false, unit: !0, variables: !16)
!16 = !{!14}
!17 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)
 ; 
!28 = !DILocalVariable(name: "inlined", scope: !15, file: !6, line: 5, type: !29) ; OOPS second DILocalVariable
!29 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "some_struct", size: 32, align: 32)

Now we have two DILocalVariable for "inlined" within the same scope. This result in assert in AsmPrinter/DwarfDebug.h:131: void llvm::DbgVariable::addMMIEntry(const llvm::DbgVariable &): Assertion `V.Var == Var && "conflicting variable"' failed.
(Full example: See: https://bugs.llvm.org/show_bug.cgi?id=33492)

In this change we prevent duplication of types so that when a metadata for DILocalVariable is cloned it will get uniqued to the same metadate node as an original variable.

Diff Detail

Repository
rL LLVM

Event Timeline

GorNishanov created this revision.Jul 6 2017, 9:52 PM
loladiro accepted this revision.Jul 7 2017, 12:44 AM

The implementation of this looks good to me. I guess I'm not entirely sure why it's necessary for that type to be distinct in the first place, but that's probably a different discussion from just fixing this bug.

lib/Transforms/Utils/CloneFunction.cpp
49 ↗(On Diff #105584)

Does this ever get called without a valid F?

This revision is now accepted and ready to land.Jul 7 2017, 12:44 AM

The implementation of this looks good to me. I guess I'm not entirely sure why it's necessary for that type to be distinct in the first place, but that's probably a different discussion from just fixing this bug.

Clang code has a comment that:

// Elements of composite types usually have back to the type, creating
// uniquing cycles.  Distinct nodes are more efficient.
lib/Transforms/Utils/CloneFunction.cpp
49 ↗(On Diff #105584)

Yes, In two places.
In llvm/lib/CodeGen/WinEHPrepare.cpp and
in llvm/lib/Transforms/Utils/LoopUnroll.cpp

I am wondering if it is just an oversight due to the F parameter having a default value of nullptr and in those two places function parameter is omitted.

Thank you for the review. Preparing to land.

This revision was automatically updated to reflect the committed changes.