This is an archive of the discontinued LLVM Phabricator instance.

Clean up debug locations for logical-and/or expressions
Needs ReviewPublic

Authored by probinson on Dec 3 2020, 1:56 PM.

Details

Reviewers
dblaikie
rnk
Summary

Attaches a more appropriate debug location to the PHIs used for the
short-circuit evaluations, and makes logical-and and logical-or
handling consistent. Also sets the appropriate debug location for all
scalar conversions, which incidentally does the right thing for
int-to-bool conversions in particular.

While this does tidy up the source locations emitted to IR, it doesn't
appear to make any difference in the final object file for simple
tests. However, it should permit re-applying D91734 and friends to
improve the debug info without exposing the regressions that prompted
the reverts in #615f63e.

Diff Detail

Unit TestsFailed

Event Timeline

probinson requested review of this revision.Dec 3 2020, 1:56 PM
probinson created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 1:56 PM
rnk added inline comments.Dec 3 2020, 2:09 PM
clang/lib/CodeGen/CGExprScalar.cpp
4293

I see this is consistent with LAnd handling above, but is there a principled reason for using an empty location here? If I was stopped on this unconditional branch instruction in the debugger, I would expect to see the location of the logical operator.

No need to change behavior in this patch, I'm just wondering if we can remove this as a followup.

I dug into the blame, and the comment about "no need" for a location comes from 2011:
http://github.com/llvm/llvm-project/commit/4d7612744f080d2c0a8639c1f5e41b691e1bba55
The reasoning seems to be that it improves the gdb test suite.

probinson added inline comments.Dec 3 2020, 2:37 PM
clang/lib/CodeGen/CGExprScalar.cpp
4293

I have no insight here. I'm willing to take it on as a follow-up exercise, after the FastISel patch goes back in.

Probably worth separating these changes/fixes/tests (looks like 3 different changes?) - at least it'd help me understand what each one of the changes does, separately (I'm especially curious about the EmitScalarConversion one and would like to better understand why the generic Expr handling for ApplyDebugLocation isn't working here, if it can be made more robust and/or if the proposed solution could be generalized a bit/moved further up in the stack). And is there an existing test file/cases these could be put near?

clang/lib/CodeGen/CGExprScalar.cpp
4293

I /think/ I remember encountering these issues independently/as well when I was working on the gdb test suite maybe a couple of years later. Hmm, guess it might've overlapped with this work.

6f2e41e0d4421ab376801bfb88d3f197a8e96994 / r128471 provides maybe a bit more context: "Do not line number entry for unconditional branches. Usually, users do not want to stop at closing '}'."

clang/lib/CodeGen/CGExprScalar.cpp