Page MenuHomePhabricator

[DebugInfo] Support for signed constants inside DIExpression
ClosedPublic

Authored by SouraVX on Mar 24 2021, 9:00 AM.

Details

Summary

Negative numbers are represented using DW_OP_consts along with signed representation
of the number as the argument.

Test case IR is generated using Fortran front-end.

Diff Detail

Event Timeline

SouraVX created this revision.Mar 24 2021, 9:00 AM
SouraVX requested review of this revision.Mar 24 2021, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 9:00 AM

Patch is stemmed from the discussions from https://reviews.llvm.org/D97311

dstenb added a subscriber: dstenb.Mar 24 2021, 9:13 AM
dstenb added inline comments.
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3052 ↗(On Diff #333015)

Why do we only care about unsigned constants here?

(And in the other cases where isConstant() is changed to isUnsignedConstant())

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
216

Can we make a isConstant() function in DIExpression that wraps these two?

llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

Why do we not allow fragmented expressions here but we do for the unsigned variant?

Thanks for review @dstenb !

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3052 ↗(On Diff #333015)

Thanks for the question! As noted in the patch summary, we are trying to be conservative here.
Motivation behind this change, Previous code made a check for isConstant which sounds/seems ambiguous to me when I introduced isSignedConstant so I took the opportunity to make the change conservatively.

dstenb added inline comments.Mar 24 2021, 9:40 AM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3052 ↗(On Diff #333015)

Okay, thanks! So all of those changes are done conservatively, rather than signed constants being unwanted there. Can we add TODO comments that say that we should investigate if signed constants should be covered also for each of these cases?

aprantl added inline comments.Mar 24 2021, 4:10 PM
llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
3236 ↗(On Diff #333015)

Based on the assertion message though, it sounds like this assertion, for example, really wnats to check for any constants?

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
216

Yes, that would be a good idea.

SouraVX updated this revision to Diff 333222.EditedMar 25 2021, 12:22 AM

Thanks @aprantl for reviewing this.
Changes:

  • Rebased to tip.
  • Addressed @dstenb review comments regarding covering fragments and added test case to excercise that.
  • Addressed @aprantl review comments of combinining check to a single function i.e isConstant.
  • Replaced most checks of isUnsignedConstant with isConstant adding appropriate test case modifications to exercise the changes.
SouraVX marked 3 inline comments as done.Mar 25 2021, 12:23 AM
SouraVX added inline comments.Mar 25 2021, 12:29 AM
llvm/include/llvm/IR/DebugInfoMetadata.h
2585

These functions can be cascaded/merged into isConstant function body(with multiple if statements). However keeping them orthogonal/independent has advantages of it's own.
One motivating example can be seen at llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:217 where this function call fulfill the intent of conveying the necessary info i.e signed or unsigned.

SouraVX updated this revision to Diff 333227.Mar 25 2021, 12:53 AM
  • Minor comment updation in isSignedConstant Function in DebugInfoMetadata.cpp
SouraVX edited the summary of this revision. (Show Details)Mar 25 2021, 12:56 AM
jmorse added a subscriber: jmorse.Mar 25 2021, 4:51 AM

Sounds good overall, with a comment inline about the test for isSignedConstant. IMO, there should be some test coverage of expressions that shouldn't be recognised as a signed constant too.

llvm/lib/IR/DebugInfoMetadata.cpp
1480

I may have mis-read, but shouldn't the end && on this line be ||? Either the zeroth element not being consts, *or* the second element not being stack value, would mean this doesn't match the constant pattern, no?

Additionally: shouldn't there be a clause for getNumElements() == 6 here? Otherwise, any six element expression with a fragment suffix will be identified a constant by this function, regardless of the first three elements.

SouraVX updated this revision to Diff 333333.Mar 25 2021, 10:05 AM

Thanks for the review @jmorse!

  • Addressed review comments by @jmorse.
  • Reinforced checks for both isUnsignedConstant and isSignedConstant.
  • Added an invalid expression test case covering isSignedConstant 6 arguments case with operand 0 and operand 2 matching.
SouraVX marked an inline comment as done.Mar 25 2021, 10:06 AM
SouraVX added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
1480

Taken care thanks!

aprantl added inline comments.Mar 26 2021, 9:54 AM
llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

It's costly to maintain two function that share 95% of their implementation. What would you think about having a single function

llvm::Optional<enum {unsigned, signed}> DIExpression::isConstant() const;

instead? Then it can serve as a drop-in replacements for call sites that don't care about the signedness and ones that do still get that information.

SouraVX updated this revision to Diff 333961.Mar 29 2021, 12:22 PM
SouraVX marked an inline comment as done.

Changes:

  • Rebased to the tip.
  • Addressed @aprantl review comments.
SouraVX marked an inline comment as done.Mar 29 2021, 12:27 PM
SouraVX added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

Thanks for the suggestion! I went a little ahead and did it in a more functional way, preserving the semantics in individual lambdas.
Please let me know, WDYT ?

aprantl added inline comments.Mar 29 2021, 2:51 PM
llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

You implemented my suggestion very literally. Ideally we would share the *implementation* of the two lambdas as well, for example, by inverting the condition and use early exists:

if ((getNumElements() == 2 && (getElement(0) != dwarf::DW_OP_consts))
  return true;
if  (getNumElements() != 3_
  return false;
if ((getElement(0) != dwarf::DW_OP_consts || (getElement(0) != dwarf::DW_OP_constu))
  return false;
return getElement(2) != dwarf::DW_OP_stack_value);

...

aprantl added inline comments.Mar 29 2021, 2:55 PM
llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

should have been
if ((getElement(0) != dwarf::DW_OP_consts && (getElement(0) != dwarf::DW_OP_constu))

SouraVX updated this revision to Diff 334137.Mar 30 2021, 6:32 AM
SouraVX marked an inline comment as done.
  • Rebased & addressed @aprantl review comments.
SouraVX updated this revision to Diff 334138.Mar 30 2021, 6:35 AM

Minor comment correction.

SouraVX marked 2 inline comments as done.Mar 30 2021, 6:39 AM
SouraVX added inline comments.
llvm/lib/IR/DebugInfoMetadata.cpp
1475–1476

I've tried to follow what you meant :)
Changes:

  • De-duplicated/factored out common code.
  • Arranged expensive checks first, this should help in short-circuiting in case of malformed expression.
aprantl added inline comments.Mar 30 2021, 9:50 AM
llvm/lib/IR/DebugInfoMetadata.cpp
1464

signed

1466

is there a missing Ofs and ) here?

1477

Shouldn't this compare getElement(0)?

SouraVX updated this revision to Diff 334208.Mar 30 2021, 10:02 AM
  • Addressed @aprantl review comments. Thanks!.
SouraVX marked 3 inline comments as done.Mar 30 2021, 10:03 AM
aprantl accepted this revision.Mar 30 2021, 10:38 AM
This revision is now accepted and ready to land.Mar 30 2021, 10:38 AM
This revision was landed with ongoing or failed builds.Mar 30 2021, 10:50 AM
This revision was automatically updated to reflect the committed changes.