[IR/DIVar] Add flag for params that have unchanged values

[djtodoro]

Summary

Introduce debug info flag that indicates that a parameter has unchanged value through the course of a function.

This info will be used for emitting DW_OP_entry_value expressions for the parameter.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

[aprantl]

Thanks! I'm assuming the idea is that this is analyzed by the language frontend?

To make sure that we're getting the design right: Could you make an argument for why it is not sufficient to deduce this information at the MIR level by looking at the DBG_VALUEs of each variable?
For example, if the first DBG_VALUE is at the beginning of the variable's lexical scope and no other DBG_VALUE for the variable exists the variable should be constant, because every other SSA value associated with the same variable should have contributed another DBG_VALUE and even if the debug info got lost there should be a DBG_VALUE undef that survived. I guess it won't be hard to come up with a counterexample...

About the semantics: Does !isNotChanged() imply that the variable may be modified and should a MIR function with a *single* DBG_VALUE for a variable the !isNotChanged() throw a verifier error because we must have dropped another DBG_VALUE? Similarly, should or could the IR Verifier check for this?

Lastly, I've been meaning to add an isConstant flag to DIFlags to support programming languages with immutable variables (such as Swift let bindings) better. An isConstant would be emitted as a DW_TAG_constant instead of DW_TAG_variable. I'm wondering if isNotChanged and isConstant are close enough that they could be merged. What do you think?

[djtodoro]

@aprantl Thanks for your comments!

I'm assuming the idea is that this is analyzed by the language frontend?

You got the idea right! :)

To make sure that we're getting the design right: Could you make an argument for why it is not sufficient to deduce this information at the MIR level by looking at the DBG_VALUEs of each variable?
For example, if the first DBG_VALUE is at the beginning of the variable's lexical scope and no other DBG_VALUE for the variable exists the variable should be constant, because every other SSA value associated with the same variable should have contributed another DBG_VALUE and even if >the debug info got lost there should be a DBG_VALUE undef that survived. I guess it won't be hard to come up with a counterexample...

Actually, here is the example.

bb.0.entry:
...
DBG_VALUE $edi, $noreg, !"a",…
$ebp = MOV32rr $edi
DBG_VALUE $ebp, $noreg, !"a"…
CALL64pcrel32 …
...

If we have a function with parameter a and it is located in a callee-clobbered register at the beginning of the function. If there is a use of the parameter in the rest of the function (after a call), there will be such move instruction (such in the example shown above) that changes the location of the parameter into some callee-saved register, even the parameter did not change its value. It will imply generating a new DBG_VALUE for variable a. By using some heuristics, this could be handled I guess and figure it out that a parameter is only moved to another location.
On IR level, it should be possible to make a verifier.

Lastly, I've been meaning to add an isConstant flag to DIFlags to support programming languages with immutable variables (such as Swift let bindings) better. An isConstant would be emitted as a DW_TAG_constant instead of DW_TAG_variable. I'm wondering if isNotChanged and isConstant >are close enough that they could be merged. What do you think?

In general, every isConstant should be IsNotChanged. Those are very similar, but I would say not exactly the same. We mark parameters as isNotChanged when its value did not changed through the course of a function (that implies its value was constant in that function, but it is not necessary declared as const). But, for example in Swift by using let keyword you declare a constant. For such cases, it makes sense to generate DW_TAG_constant.

[aprantl]

Yeah, that makes sense. It would be very hard to analyze this without either false positives or a ton of false negatives. I also see that while every constant is trivially not changed, the opposite is not true.

Could you please add a round-trip test (like or perhaps even in test/Assembler/debug-info.ll )?

include/llvm/IR/DebugInfoFlags.def

62

The attribute it named VariableNotChanged, but IIUC, this can only be applied to function arguments? Should it be called ArgumentNotModified instead?

[djtodoro]

@aprantl I agree. Thanks for the comment!

Could you please add a round-trip test (like or perhaps even in test/Assembler/debug-info.ll )?

Sure.

Besides this, there is a test case in clang's part for generating the flag. For now, we restricted generating it only in the case of '-femit-param-entry-values' option.

include/llvm/IR/DebugInfoFlags.def

62

Since we are using it only for arguments now, it sounds reasonable to me.

We put VariableNotChanged in order to leave a chance for it to be used at some other places in compiler for whether local variables or function arguments. Either for extension of 'tracking parameter's entry value' feature or somewhere else. For now, using ArgumentNotModified is better.

[djtodoro]

• Rename: VariableNotChanged ===> ArgumentNotModified
• Add test case in test/Assembler/debug-info.ll

[aprantl]

LGTM. One last request: Could you please add (either to the doxygen comment of isArgNotModified or to LangRef.rst or SourceLevelDebugging.rst) an explanation of the semantics of the flag. Ie.: What it is used for and under what conditions a frontend should generate it?

[djtodoro]

LGTM. One last request: Could you please add (either to >the doxygen comment of isArgNotModified or to >LangRef.rst or SourceLevelDebugging.rst) an >explanation of the semantics of the flag. Ie.: What it is >used for and under what conditions a frontend should >generate it?

@aprantl Sure. Thanks a lot!

[djtodoro]

• Add documentation for the flag

[djtodoro]

@aprantl I just updated the diff with documentation. Please let me know what do you think.

I didn't put explicitly how we generate this at the moment in front end, because (for now) it is restricted under the option for the whole new feature (-femit-param-entry-values).

[aprantl]

docs/LangRef.rst

4736

These flags encode various properties of DINodes.

4738

The `ArgumentNotModified` flag marks a function argument whose value...

4739

through the course or throughout

4740

How about: This flag is used to decide whether a DW_OP_entry_value can be used in a location description after the function prologue.

4742

The language frontend is expected to compute this property for each DILocalVariable.

[djtodoro]

• Adjust documentation

[aprantl]

Looking good, thanks!

[djtodoro]

-Rebase
-Add setIsNotModified()

[aprantl]

Looks still good.

[djtodoro]

Thanks!

[djtodoro]

-Rebase

bindings/go/llvm/dibuilder.go

const (
   FlagArtificial
   FlagExplicit
   FlagPrototyped
   FlagObjcClassComplete
   FlagObjectPointer
   FlagVector
   FlagStaticMember
   FlagIndirectVariable
+  FlagArgumentNotModified
 )
 
 type DwarfLang uint32
 
 const (
   // http://dwarfstd.org/ShowIssue.php?issue=101014.1&type=open
   DW_LANG_Go DwarfLang = 0x0016
 )

docs/LangRef.rst

.. code-block:: text
     !0 = !DIExpression(DW_OP_deref)
     !1 = !DIExpression(DW_OP_plus_uconst, 3)
     !1 = !DIExpression(DW_OP_constu, 3, DW_OP_plus)
     !2 = !DIExpression(DW_OP_bit_piece, 3, 7)
     !3 = !DIExpression(DW_OP_deref, DW_OP_constu, 3, DW_OP_plus, DW_OP_LLVM_fragment, 3, 7)
     !4 = !DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef)
     !5 = !DIExpression(DW_OP_constu, 42, DW_OP_stack_value)
 
+DIFlags
+"""""""""""""""
+
+These flags encode various properties of DINodes.
+
+The `ArgumentNotModified` flag marks a function argument whose value
+is not modified throughout of a function. This flag is used to decide
+whether a DW_OP_entry_value can be used in a location description
+after the function prologue. The language frontend is expected to compute
+this property for each DILocalVariable. The flag should be used
+only in optimized code.
+
 DIObjCProperty
 """"""""""""""
 
 ``DIObjCProperty`` nodes represent Objective-C property nodes.
 
 .. code-block:: text
 
     !3 = !DIObjCProperty(name: "foo", file: !1, line: 7, setter: "setFoo",

include/llvm/IR/DebugInfoFlags.def

 // 15 was formerly ExternalTypeRef, but this was never used.
 HANDLE_DI_FLAG((1 << 15), Reserved)
 HANDLE_DI_FLAG((1 << 16), SingleInheritance)
 HANDLE_DI_FLAG((2 << 16), MultipleInheritance)
 HANDLE_DI_FLAG((3 << 16), VirtualInheritance)
 HANDLE_DI_FLAG((1 << 18), IntroducedVirtual)
 HANDLE_DI_FLAG((1 << 19), BitField)
 HANDLE_DI_FLAG((1 << 20), NoReturn)
+HANDLE_DI_FLAG((1 << 21), ArgumentNotModified)
 HANDLE_DI_FLAG((1 << 22), TypePassByValue)
 HANDLE_DI_FLAG((1 << 23), TypePassByReference)
 HANDLE_DI_FLAG((1 << 24), EnumClass)
 HANDLE_DI_FLAG((1 << 25), Thunk)
 HANDLE_DI_FLAG((1 << 26), NonTrivial)
 HANDLE_DI_FLAG((1 << 27), BigEndian)
 HANDLE_DI_FLAG((1 << 28), LittleEndian)
 HANDLE_DI_FLAG((1 << 29), AllCallsDescribed)
 
 // To avoid needing a dedicated value for IndirectVirtualBase, we use
 // the bitwise or of Virtual and FwdDecl, which does not otherwise
 // make sense for inheritance.
 HANDLE_DI_FLAG((1 << 2) | (1 << 5), IndirectVirtualBase)
 
 #ifdef DI_FLAG_LARGEST_NEEDED
 // intended to be used with ADT/BitmaskEnum.h
 // NOTE: always must be equal to largest flag, check this when adding new flag

include/llvm/IR/DebugInfoMetadata.h

public:
 
   bool isParameter() const { return Arg; }
   unsigned getArg() const { return Arg; }
   DIFlags getFlags() const { return Flags; }
 
   bool isArtificial() const { return getFlags() & FlagArtificial; }
   bool isObjectPointer() const { return getFlags() & FlagObjectPointer; }
 
+  /// Check that an argument is unmodified.
+  bool isNotModified() const { return getFlags() & FlagArgumentNotModified; }
+  /// Set the flag if an argument is unmodified.
+  void setIsNotModified() { Flags |= FlagArgumentNotModified; }
+
   /// Check that a location is valid for this variable.
   ///
   /// Check that \c DL exists, is in the same subprogram, and has the same
   /// inlined-at location as \c this.  (Otherwise, it's not a valid attachment
   /// to a \a DbgInfoIntrinsic.)
   bool isValidLocationForIntrinsic(const DILocation *DL) const {
     return DL && getScope()->getSubprogram() == DL->getScope()->getSubprogram();
   }

test/Assembler/debug-info.ll

 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder %s
 
-; CHECK: !named = !{!0, !0, !1, !2, !3, !4, !5, !6, !7, !8, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !27, !28, !29, !30, !31, !32, !33, !34, !35, !36, !37, !38, !39}
-!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !28, !29, !30, !31, !32, !33, !34, !35, !36, !37, !38, !39, !40, !41, !42}
+; CHECK: !named = !{!0, !0, !1, !2, !3, !4, !5, !6, !7, !8, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !27, !28, !29, !30, !31, !32, !33, !34, !35, !36, !37, !38, !39, !40, !41}
+!named = !{!0, !1, !2, !3, !4, !5, !6, !7, !8, !9, !10, !11, !12, !13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !28, !29, !30, !31, !32, !33, !34, !35, !36, !37, !38, !39, !40, !41, !42, !43, !44}
 
 ; CHECK:      !0 = !DISubrange(count: 3)
 ; CHECK-NEXT: !1 = !DISubrange(count: 3, lowerBound: 4)
 ; CHECK-NEXT: !2 = !DISubrange(count: 3, lowerBound: -5)
 !0 = !DISubrange(count: 3)
 !1 = !DISubrange(count: 3, lowerBound: 0)
 
 !2 = !DISubrange(count: 3, lowerBound: 4)

 ; CHECK-NEXT: !37 = !DIFile(filename: "file", directory: "dir", checksumkind: CSK_MD5, checksum: "3a420e2646916a475e68de8d48f779f5", source: "int source() { }\0A")
 !39 = !DIFile(filename: "file", directory: "dir", source: "int source() { }\0A")
 !40 = !DIFile(filename: "file", directory: "dir", checksumkind: CSK_MD5, checksum: "3a420e2646916a475e68de8d48f779f5", source: "int source() { }\0A")
 
 ; CHECK-NEXT: !38 = !DIBasicType(name: "u64.be", size: 64, align: 1, encoding: DW_ATE_unsigned, flags: DIFlagBigEndian)
 ; CHECK-NEXT: !39 = !DIBasicType(name: "u64.le", size: 64, align: 1, encoding: DW_ATE_unsigned, flags: DIFlagLittleEndian)
 !41 = !DIBasicType(name: "u64.be", size: 64, align: 1, encoding: DW_ATE_unsigned, flags: DIFlagBigEndian)
 !42 = !DIBasicType(name: "u64.le", size: 64, align: 1, encoding: DW_ATE_unsigned, flags: DIFlagLittleEndian)
+
+; CHECK-NEXT: !40 = distinct !DISubprogram(name: "fn", scope: {{.*}}, file: {{.*}}, spFlags: 0)
+; CHECK-NEXT: !41 = !DILocalVariable(name: "Name", arg: 1, scope: {{.*}}, file: {{.*}}, line: 13, type: {{.*}}, flags: DIFlagArgumentNotModified)
+!43 = distinct !DISubprogram(name: "fn", scope: !12, file: !12, spFlags: 0)
+!44 = !DILocalVariable(name: "Name", arg: 1, scope: !43, file: !12, line: 13, type: !7, flags: DIFlagArgumentNotModified)