This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Event Timeline

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

Or we don't set debugloc for PHI.

496

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.Apr 22 2021, 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.Apr 23 2021, 1:24 AM
This revision is now accepted and ready to land.Apr 23 2021, 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.Apr 26 2021, 7:35 AM
Nuullll updated this revision to Diff 340741.Apr 27 2021, 12:29 AM
Nuullll marked an inline comment as done.

Removed useless attributes in the test.

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

Thanks!

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

Thanks for this!
I can commit this for you (with your name in the 'Patch by' sign) in the case you don't have commit access yet?

Ah, this is my first time trying to make a llvm contribution.
What shall I do next to get my patch landed?

Thanks for this!
I can commit this for you (with your name in the 'Patch by' sign) in the case you don't have commit access yet?

That would be great! I don't think I have commit access.