Page MenuHomePhabricator

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

Authored by djtodoro on Feb 11 2019, 3:19 AM.

Details

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

Diff Detail

Event Timeline

djtodoro created this revision.Feb 11 2019, 3:19 AM
djtodoro added a project: debug-info.
djtodoro added a subscriber: petarj.

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?

@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.

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 marked an inline comment as done.Feb 13 2019, 2:35 AM

@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 updated this revision to Diff 186609.Feb 13 2019, 2:40 AM
  • Rename: VariableNotChanged ===> ArgumentNotModified
  • Add test case in test/Assembler/debug-info.ll
aprantl accepted this revision.Feb 13 2019, 8:53 AM

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?

This revision is now accepted and ready to land.Feb 13 2019, 8:53 AM

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 updated this revision to Diff 187189.Feb 17 2019, 10:36 PM
  • Add documentation for the flag

@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 added inline comments.Feb 18 2019, 9:22 AM
docs/LangRef.rst
4714

These flags encode various properties of DINodes.

4716

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

4717

through the course or throughout

4718

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.

4720

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

djtodoro updated this revision to Diff 187317.Feb 19 2019, 12:04 AM
  • Adjust documentation

Looking good, thanks!

djtodoro updated this revision to Diff 195191.Apr 15 2019, 8:29 AM

-Rebase
-Add setIsNotModified()

Looks still good.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 4:20 AM