Page MenuHomePhabricator

[Transforms][Debugify] Fix "Missing line" false alarm on PHI nodes
AcceptedPublic

Authored by Nuullll on Tue, Apr 13, 10:41 PM.

Diff Detail

Event Timeline

Nuullll created this revision.Tue, Apr 13, 10:41 PM
Nuullll requested review of this revision.Tue, Apr 13, 10:41 PM
xiangzhangllvm added inline comments.Thu, Apr 22, 2:06 AM
llvm/lib/Transforms/Utils/Debugify.cpp
140

Or we don't set debugloc for PHI.

495

Here also ship PHI

@vks, do you know why we set PHI with debug_loc but not check it before?

vsk resigned from this revision.Thu, Apr 22, 1:57 PM
vsk edited reviewers, added: aprantl, djtodoro, probinson; removed: vsk.

Context: In D59417 (see also llvm.org/PR37964), there was some debate about whether it's worthwhile to assign a line 0 location to a phi instruction which would otherwise not have a location. The consensus seemed to be: no, it's not worth it. So D75242 was spun off to suppress the false positive in debugify. It looks like D75242 had the unexpected/unfortunate side-effect of introducing a new FP: any unique, preserved DebugLoc attached to a phi instruction because "missing".

My two cents is that a total ban on attaching debug location/intrinsic info to phi instructions isn't the best way to fix this. That would lead to a loss in test coverage (we do see cases where passes copy debug locations from phi instructions, or where debug intrinsics refer to phi instructions). I think the current version of this patch (skip the warning on phi instructions) does the right thing. That said I'm no longer very active in debug info reviews so I'll defer to other reviewers.

Thanks for your detailed explanation, In my eyes, D75242 eliminated uncommon "warnings" (only pass generate PHI node), but generate common "warnings" (all pass with PHI node ).
Let me first +1 for this patch if no other reviewers object.
LGTM

xiangzhangllvm accepted this revision.Fri, Apr 23, 1:24 AM
This revision is now accepted and ready to land.Fri, Apr 23, 1:24 AM

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?
Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

llvm/test/DebugInfo/debugify-ignore-phi.ll
19

I think we can remove the attributes.

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?

Yes

Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

Of course we can. But this warning just for pass generate PHI, we usually ignore warnings if they are make sense. I think the reason @Nuullll write this patch is just that "PHI existed in too many passes, but very few pass generate PHI"

So this patch will remove reporting warnings of missing debug loc to PHIs completely, right?

Yes

Can we at least introduce an option to keep checking the PHIs, because there are cases it can be a real issue detected by debugify.

Of course we can. But this warning just for pass generate PHI, we usually ignore warnings if they are make sense. I think the reason @Nuullll write this patch is just that "PHI existed in too many passes, but very few pass generate PHI"

got it, thanks. This lgtm.

djtodoro accepted this revision.Mon, Apr 26, 7:35 AM
Nuullll updated this revision to Diff 340741.Tue, Apr 27, 12:29 AM
Nuullll marked an inline comment as done.

Removed useless attributes in the test.

djtodoro accepted this revision.Tue, Apr 27, 12:41 AM

Thanks!