[PATCH] D115714: [Debugify] Limit number of processed instructions for original mode

[Nikola Tesic via Phabricator via llvm-commits]

ntesic added a comment.

Hi @StephenTozer, sorry for the delay, I was AFK.
Thanks for your comments.

In D115714#3243396 <https://reviews.llvm.org/D115714#3243396>, @StephenTozer wrote:

> Performance seems like a serious issue when working with large projects here, but I have some questions/thoughts about this approach:

Definitely, and large projects are precious to us, for catching Debug Info Losses in the LLVM pipeline.

> Is there any good reason to have an option to set the limit to zero? Unless I'm missing something, that would be equivalent to just not running debugify at all, which seems like a redundant option to have when debugify is an optional flag to begin with.

The zero limit was useful for comparing build times during development of these patches, but I agree that it is not very useful for users.

> More generally, it's probably good to have an optional limit for OriginalDIMode, but I think it would be better if the limit could be passed as an integer instead of an enum. While 10000 may be a good default value for the builds you mentioned in the description, this is only with respect to your hardware setup and time constraints, and even then may be different for other builds (or even different optimization pipelines). Other users may have a higher or lower limit on the number of instructions they can afford to have processed by debugify, and should be able to specify this exactly via command line.

Explicitly setting the instruction limit number instead of using preset number, means the user should know its system constraints. This probably makes sense.

> Also this is just my opinion and so I'd like to see what other reviewers think, but personally I think it would be best if the default setting was unlimited (so the limit is purely optional). When we're talking about metric-gathering, I think it's best to have the program do exactly what it says on the tin/what the user would expect it to do, and silently skipping instructions unless you pass an additional flag seems like potentially harmful unexpected behaviour. The documentation should be updated to include this option so that if a user does encounter performance issues then they will know that the option is there, but they will never unknowingly get incomplete results because they didn't know about this flag.

This patch came from our downstream use case, and in the general use case, all your comments make more sense. Thanks again!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115714/new/

https://reviews.llvm.org/D115714