Page MenuHomePhabricator

[CGP] Reset the debug location when promoting zext(s)
ClosedPublic

Authored by davide on Mon, Jun 8, 3:54 PM.

Diff Detail

Event Timeline

davide created this revision.Mon, Jun 8, 3:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 8, 3:55 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

That's a pretty long test case for a one-line patch; there's no existing zext testcase where you can just add !dbg metadata?

Let me try putting an assertion with the patch and see what I can do.

Still looking for a reduced tescase but I noticed another bug, so I put another review online:
https://reviews.llvm.org/D81879

I tried really hard to reduce this one but I failed. I guess I can either drop this review or I'm happy to hear suggestions. @probinson

I might have found a way to reduce :)

davide updated this revision to Diff 271233.Tue, Jun 16, 3:38 PM

Updated diff/reduced testcase/found another bug/added context.

vsk added inline comments.Tue, Jun 16, 3:41 PM
llvm/lib/CodeGen/CodeGenPrepare.cpp
5809

Might be worth moving parts of this comment near the TypePromotionTransaction definition, to justify why those clear the IRBuilder debug loc?

llvm/test/DebugInfo/X86/zextload.ll
26

{{$}}?

davide updated this revision to Diff 271234.Tue, Jun 16, 3:50 PM

Address Vedant's feedback.

vsk accepted this revision.Tue, Jun 16, 4:18 PM

Lgtm with minor nitpicks inline, thanks!

llvm/lib/CodeGen/CodeGenPrepare.cpp
2652

To quibble: s/would/could/ might be more fair, since we're conservatively dropping the location just in case the created instruction gets moved around after the fact -- it's just that this tends to happen a lot :). Also, maybe moving this to the TypePromotionAction class definition would serve as a more general warning?

This revision is now accepted and ready to land.Tue, Jun 16, 4:18 PM
probinson accepted this revision.Wed, Jun 17, 8:52 AM

Definitely better, thanks!

davide closed this revision.Wed, Jun 17, 11:13 AM

Committed

commit 1cbaf847ab84f37d9044d867b5ed8ae646c6ebc9 (HEAD -> master, origin/master, origin/HEAD)
Author: Davide Italiano <ditaliano@apple.com>
Date:   Wed Jun 17 10:29:47 2020 -0700

    [CGP] Reset the debug location when promoting zext(s).
    
    When the zext gets promoted, it used to retain the original location,
    which pessimizes the debugging experience causing an unexpected
    jump in stepping at -Og.
    
    Fixes https://bugs.llvm.org/show_bug.cgi?id=46120 (which also
    contains a full C repro).
    
    Differential Revision:  https://reviews.llvm.org/D81437