Page MenuHomePhabricator

expose debugify as an LLVM option in clang
Needs ReviewPublic

Authored by tyb0807 on Jul 13 2018, 7:59 AM.

Details

Summary

Hello,

As discussed in https://bugs.llvm.org/show_bug.cgi?id=37792.

Thanks for your review

Diff Detail

Event Timeline

tyb0807 created this revision.Jul 13 2018, 7:59 AM

Thanks for working on this! I've not reviewed the code, but I did try applying the patch and making use of it, and it seems to work perfectly so far for my use-case of being able to automatically enable automatic debugify checking over large codebases in build systems that I don't necessarily have control of :)

I built a medium sized PS4 codebase with "clang -O3 -mllvm -debugify-each" and piped stderr into a file, where I was able to generate the following stats. Obviously there will be some level of duplication due to linkonce functions, but looking through the data, it seems that the vast majority of error lines are unique so I don't think that's a big factor skewing the results.

$ cat err2.txt | grep FAIL | sort | uniq -c | sort
      2 CheckFunctionDebugify [MemCpy Optimization]: FAIL
      8 CheckFunctionDebugify [Tail Call Elimination]: FAIL
     17 CheckFunctionDebugify [Value Propagation]: FAIL
     23 CheckModuleDebugify [Global Variable Optimizer]: FAIL
     53 CheckFunctionDebugify [Dead Store Elimination]: FAIL
     76 CheckFunctionDebugify [Call-site splitting]: FAIL
    100 CheckFunctionDebugify [MergedLoadStoreMotion]: FAIL
    126 CheckFunctionDebugify [Constant Hoisting]: FAIL
    130 CheckFunctionDebugify [Loop Vectorization]: FAIL
    142 CheckFunctionDebugify [Reassociate expressions]: FAIL
    185 CheckModuleDebugify [Dead Argument Elimination]: FAIL
    385 CheckFunctionDebugify [SROA]: FAIL
    445 CheckFunctionDebugify [Merge disjoint stack slots]: FAIL
    555 CheckFunctionDebugify [Simplify the CFG]: FAIL
    584 CheckFunctionDebugify [Global Value Numbering]: FAIL
    727 CheckFunctionDebugify [CodeGen Prepare]: FAIL
    845 CheckFunctionDebugify [Jump Threading]: FAIL
   1761 CheckFunctionDebugify [Canonicalize natural loops]: FAIL
   2429 CheckFunctionDebugify [Combine redundant instructions]: FAIL
   8042 CheckFunctionDebugify [Loop-Closed SSA Form Pass]: FAIL

Based on this data, I'll attempt to produce a small test-case that demonstrates an LCSSA failure and raise a bug report.

tyb0807 added a comment.EditedJul 17 2018, 2:30 PM

Thanks for your comment Greg. Please feel free to tell me if you need anything else I'll try to update the patch.

I will try to look at this tomorrow if @vsk doesn't. Passes are not really my thing but as it's based on the existing opt code it should not be too hard to review.

vsk added a comment.Jul 17 2018, 3:51 PM

First off, @tyb0807, I really appreciate the work you've been doing on LLVM.

With that said, I think this is trying to solve the wrong problem, and I don't think it makes sense to proceed.

There's already a way to find targeted test cases for debug info bugs with opt (see https://gramanas.github.io/posts/finding-debuginfo-loss/). I don't think it makes sense to land a change this invasive to make that process marginally easier. Especially not when there's a more general way to collect IR from a build system using a simple wrapper (see e.g https://github.com/vedantk/scripts/blob/master/generate-ir-cc).

We're bottlenecked by our ability to *fix* -- not simply to find -- these bugs.

In D49297#1165863, @vsk wrote:

First off, @tyb0807, I really appreciate the work you've been doing on LLVM.

With that said, I think this is trying to solve the wrong problem, and I don't think it makes sense to proceed.

That's fair, although I somewhat disagree and I'll say that for our workflow it's a bit more than marginal in terms of how much easier it is, but one thing I'd really like to get to the bottom of before rejecting this approach entirely are the following failures above:

445 CheckFunctionDebugify [Merge disjoint stack slots]: FAIL
727 CheckFunctionDebugify [CodeGen Prepare]: FAIL

I've had a background process running on my PC hitting some preprocessed game code files pretty hard for a couple of weeks now. It's produced small bug reports for a number of passes, but the above failures don't get spotted with the clang piped into opt approach. I've unfortunately been temporarily waylaid by another project for the last couple of months or so (otherwise I'd already be contributing fixes for the bugs I've raised) so I've not had a chance to investigate whether these are genuine failures or false positives. The big question for me, is does this increase the potential coverage?

vsk added a comment.Jul 17 2018, 4:54 PM
445 CheckFunctionDebugify [Merge disjoint stack slots]: FAIL
727 CheckFunctionDebugify [CodeGen Prepare]: FAIL

I've had a background process running on my PC hitting some preprocessed game code files pretty hard for a couple of weeks now. It's produced small bug reports for a number of passes, but the above failures don't get spotted with the clang piped into opt approach. [...] The big question for me, is does this increase the potential coverage?

By default opt doesn't run -codegenprepare in its -OX pipelines, but it does supporting testing it. See e.g D47282.

The -stack-coloring pass lives in a weird space: it's a machine function pass which runs after isel, but it modifies IR. I don't think debugify operates on a very useful level at this point.

Hi @vsk,

Thank you for your comment. I agree with what you say, however, I think you may understand that we should have had this discussion before this patch in https://bugs.llvm.org/show_bug.cgi?id=37792 😭

Anyway, I think we can still reuse part of the logic of this patch to extend the -verify-each mode, so that it also is enabled after each module/function pass? What do you think? Do you still need this feature of -verify-each?

vsk added a comment.Jul 18 2018, 10:51 AM

Hi @vsk,

Thank you for your comment. I agree with what you say, however, I think you may understand that we should have had this discussion before this patch in https://bugs.llvm.org/show_bug.cgi?id=37792 😭

Keep in mind that folks can't respond to everything they're CC'd on. 'Would-be-nice' feature requests like this one necessarily receive less attention. To save time in the future, let people know what you're working on. Assigning a bugzilla PR to yourself, sending out a RFC or an implementation plan on llvm-dev, or even pinging people on IRC are all good ways to do that.

Anyway, I think we can still reuse part of the logic of this patch to extend the -verify-each mode, so that it also is enabled after each module/function pass? What do you think? Do you still need this feature of -verify-each?

If you're interested in adding/changing a -verify-each mode in clang/opt, I suggest sending out a RFC. That allows more people to participate in the discussion.