Page MenuHomePhabricator

[CodeGen] Fix bugs in LiveDebugVariables when debug labels are generated.
ClosedPublic

Authored by HsiangKai on Nov 12 2018, 11:21 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

HsiangKai created this revision.Nov 12 2018, 11:21 PM
aprantl added inline comments.Nov 13 2018, 4:48 PM
lib/CodeGen/LiveDebugVariables.cpp
338 ↗(On Diff #173814)

Please don't repeat the function name in the Doxygen comment. I know the other code in this file does this, but that should be fixed, too :-)

415 ↗(On Diff #173814)

Would you mind doing an NFC commit that just fixes up all the doxygen comments in this file?

We generally use \ instead of @.

529 ↗(On Diff #173814)

Can't this code be shared with the DBG_VALUE printing function?

1423 ↗(On Diff #173814)

range-based for?

HsiangKai added inline comments.Nov 14 2018, 10:18 PM
lib/CodeGen/LiveDebugVariables.cpp
415 ↗(On Diff #173814)

I create a NFC commit in https://reviews.llvm.org/D54568

dstenb added a subscriber: dstenb.Nov 15 2018, 4:16 AM
dstenb added inline comments.
lib/CodeGen/LiveDebugVariables.cpp
501 ↗(On Diff #174161)

The cast will assert if Node is not a DILocalVariable, so I don't think it's possible to use this function with DILabels? I think you want dyn_cast_or_null (or rather dyn_cast, if Node is expected to be non-null).

vext01 added a subscriber: vext01.Nov 15 2018, 7:47 AM

This patch doesn't seem to apply. Does it need regenerating?

vext01@bencher8:~/source/llvm$ patch -Ep0 <  D54465.diff
patching file lib/CodeGen/LiveDebugVariables.cpp
Hunk #2 FAILED at 340.
Hunk #3 succeeded at 352 with fuzz 1 (offset -5 lines).
Hunk #4 FAILED at 372.
Hunk #5 succeeded at 395 (offset -9 lines).
Hunk #6 succeeded at 441 (offset -9 lines).
Hunk #7 succeeded at 466 (offset -9 lines).
Hunk #8 succeeded at 487 (offset -9 lines).
Hunk #9 succeeded at 531 (offset -9 lines).
Hunk #10 succeeded at 615 (offset -9 lines).
Hunk #11 succeeded at 651 (offset -9 lines).
Hunk #12 succeeded at 1291 (offset -7 lines).
Hunk #13 succeeded at 1348 (offset -7 lines).
2 out of 13 hunks FAILED -- saving rejects to file lib/CodeGen/LiveDebugVariables.cpp.rej
The next patch would create the file test/CodeGen/Generic/live-debug-label.ll,
which already exists!  Assume -R? [n]
wolfgangp added inline comments.
test/CodeGen/Generic/live-debug-label.ll
2 ↗(On Diff #174161)

Generally it is a good idea to add the original C++ source code as a comment, along with the command on how the IR has been generated.

HsiangKai updated this revision to Diff 174310.Nov 15 2018, 5:19 PM

This patch doesn't seem to apply. Does it need regenerating?

vext01@bencher8:~/source/llvm$ patch -Ep0 <  D54465.diff
patching file lib/CodeGen/LiveDebugVariables.cpp
Hunk #2 FAILED at 340.
Hunk #3 succeeded at 352 with fuzz 1 (offset -5 lines).
Hunk #4 FAILED at 372.
Hunk #5 succeeded at 395 (offset -9 lines).
Hunk #6 succeeded at 441 (offset -9 lines).
Hunk #7 succeeded at 466 (offset -9 lines).
Hunk #8 succeeded at 487 (offset -9 lines).
Hunk #9 succeeded at 531 (offset -9 lines).
Hunk #10 succeeded at 615 (offset -9 lines).
Hunk #11 succeeded at 651 (offset -9 lines).
Hunk #12 succeeded at 1291 (offset -7 lines).
Hunk #13 succeeded at 1348 (offset -7 lines).
2 out of 13 hunks FAILED -- saving rejects to file lib/CodeGen/LiveDebugVariables.cpp.rej
The next patch would create the file test/CodeGen/Generic/live-debug-label.ll,
which already exists!  Assume -R? [n]

Sorry for that.
I have created another patch to refine doxygen document first in https://reviews.llvm.org/D54568.
You should apply it first, then apply this patch.

HsiangKai updated this revision to Diff 174553.Nov 18 2018, 6:13 PM
probinson added inline comments.Dec 3 2018, 12:58 PM
lib/CodeGen/LiveDebugVariables.cpp
409 ↗(On Diff #174553)

The only call that I see to getUserLabel is in handleDebugLabel where the return value is not used. Am I missing something? If not, perhaps this should be a void method (with a different name, maybe addUserLabel).

HsiangKai updated this revision to Diff 176608.Dec 4 2018, 5:14 AM
dstenb added inline comments.Dec 17 2018, 1:22 AM
lib/CodeGen/LiveDebugVariables.cpp
1400 ↗(On Diff #176608)

Should this be called emitDebugLabel instead, given that we only emit one label per UserLabel?

HsiangKai updated this revision to Diff 178579.Dec 17 2018, 7:01 PM
dstenb added a comment.Jan 8 2019, 7:57 AM

This looks good to me, but I don't think I can give an official L-G-T-M.

Have you addressed all of the reviewers' comments?

test/CodeGen/Generic/live-debug-label.ll
35 ↗(On Diff #178579)

In general I think it is preferred to have the RUN: lines at the top of the file.

A few more comments.

lib/CodeGen/LiveDebugVariables.cpp
589 ↗(On Diff #178579)

The phrasing of the comment makes it sound like an assertion, rather than a description of the purpose of the loop. Maybe:
Nothing to do if Label already exists in userLabels.

678 ↗(On Diff #178579)

addUserLabel is short and not used anywhere else, so I think it would be preferable to simply put that code inline here and not have the extra method.

test/CodeGen/Generic/live-debug-label.ll
40 ↗(On Diff #178579)

These CHECK-LABEL directives can simply be CHECK directives.

HsiangKai updated this revision to Diff 181165.Jan 10 2019, 2:50 PM
This revision is now accepted and ready to land.Jan 14 2019, 8:02 AM
This revision was automatically updated to reflect the committed changes.