@CarlosAlbertoEnciso should review this ideally for approving, but I'm just nitpicking on a long line.

Thanks all for taking a look! I haven't had a chance to get back to this yet due to other work landing on my plate, but I intend to revisit this and implement the requested changes ASAP.

In D72374#1809115, @AlexanderLanin wrote:

The only difference is the git version in the end:

(quoting Jeremy's on-list reply)

In D65859#1631409, @RKSimon wrote:

I'm happy to remove the push/pull if reviewers prefer - its just a personal style preference tbh

I've not tested, but it seems reasonable. The only other "#pragma warning" I came across (in the neighbouring llvm/unittests/Support/AlignOfTest.cpp) doesn't bother with a push/pop and just disables the warnings at file scope. I'm not sure if there's any reason to push/pop across the entire file unless we think we might want to ever include this cpp file inside another one?

In D64589#1604206, @RKSimon wrote:

LGTM - cheers!

I've tried the latest update and it seems to work well for me. The only suggestion I'd make at this stage is adding a message like (did you mean --check-prefixes?) to the end of the warning if a comma is spotted in the prefix string.

In D61680#1495085, @lebedev.ri wrote:

Hm, can -debugify be run by llc?

In D60831#1472303, @aprantl wrote:

In D60831#1472296, @jmellorcrummey wrote:

As the lead of a project building profiling tools, I am strongly against having any instructions map to line 0.

This is probably not what you meant, but for completeness I feel like I should point out that there are many legitimate situations where LLVM generates a line 0 location. The most prominent example is instruction merging: Since both LLVM IR and DWARF currently require each PC address to map to exactly one source location, LLVM's will insert a line 0 location when it merges two instructions with distinct source locations. I can't speak for profiling, but at least on the debugger side, the consensus is that potentially misleading information is worse than no information, because if there is no way to distinguish "always correct" from "maybe correct" information, the user can't trust any information.

In D59431#1436497, @aprantl wrote:

"at the start of compilation" really means in mem2reg, at least for Clang. As you pointed out, there is no way to preserve the dbg.value in the exit block in mem2reg without introducing an phi node, but doing so would change codegen, which is not an option.

LGTM with one minor nit.

LGTM.

In D54187#1290282, @probinson wrote:

+gbedwell

Just to throw the idea out there, why not abandon debuginfo-tests entirely and try using Dexter for this. Dexter's design center is debug-info quality, not correctness, but it already knows how to drive several different debuggers on multiple platforms.

Oh drat. Well, I guess maybe this does manage to fall into the area of undefined behaviour then. Weirdly, that didn't happen on my local system, although I did see something similar when setting the PreferredToolArchitecture environment variable for the environment that CMake spawns in, but not in the global environment when running the VS IDE (CMAKE_C[XX]_COMPILER would not match the actual compiler being invoked). Thanks for taking a look anyway!

Another iteration :)

I find the CMake documentation very unclear here. On the related page for https://cmake.org/cmake/help/latest/variable/CMAKE_GENERATOR_TOOLSET.html it says:

In D52831#1255765, @rnk wrote:

Running cmd.exe is probably even less supported than running bash. If someone is accidentally using execute_external=True, I'd prefer it if they kept using bash and not cmd. So, I'd stick with the first patch.

Thanks both for reviewing. Given that it appears that there doesn't, as I'd worried, appear to be some special hidden use-case for bash as an external shell on Windows, but is more of a historical artifact, I'm going to propose an alternative approach. To keep the code all in the same review, and as it's in a completely separate file, I've just added it to this review in the same patch instead. To be clear, I'm talking about committing only one of these files, not both.

Rebase on top of D52560 which improves the block analysis significantly.

I've chatted with @andreadb and @RKSimon offline a bit. It would be nice to try and avoid some of the obviously duplicated blocks in the tests. For example, the added blocks in option-all-stats-1.s above. That's not actually an issue related to this specific patch, and the generated tests are still valid, but sometimes look a bit messy.

Rebased.

Modulo my nit above about the error message text, this LGTM.

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.

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 :)

In D49003#1154631, @vsk wrote:

At a high-level is it fine to use CSV here? I've found it really convenient because Google Sheets, Numbers, python scripts etc. can ingest the data easily.

This is really nice. I agree it will be really useful, albeit with a minor concern about false positives in the stats themselves due to DCE transforms like what we ran into in https://llvm.org/PR37942 .

This change would be really really useful for addressing http://llvm.org/PR37871 so I'd like to have this.

Forgot to add llvm-commits

In D48276#1135165, @lebedev.ri wrote:

It's a bit hard to review, the patch misses context (-U99999).

In D48209#1133500, @andreadb wrote:

Given that Clement (and presumably Greg) are okay with this change, then I won't oppose it. Sorry for being pedantic.

In D48209#1133474, @lebedev.ri wrote:

Actually make ninja check-llvm-tools-llvm-mca pass, as suggested/noted by @gbedwell

In D47796#1123026, @lattner wrote:

How about changing it to "LLVM Compiler Infrastructure". Accepted with that tweak, thanks!

LGTM. Thanks!

Thanks for the patch! Annoying that my git client never complained about this one to me.

LGTM too :)

minor update to formatting to get rid of blank lines around single check lines.

Delayed ping. Apologies for the delay. I started paternity leave shortly after starting this review, and never got a chance to come back to it until now.

In D45787#1071943, @RKSimon wrote:

I've raised PR37176 to track this

What's the more general plan for regression testing exegesis? It looks like the issue with pfm failing to match counters was a regression introduced by rL329675 due to the trailing commas. It would be nice to be able to have regression tests for this sort of thing, especially as it's under active development so the churn is likely to be quite high along with the risk of introducing regressions. I know it gets complicated due to the libpfm dependency and the fact it's CPU specific, but it should still be a solvable problem.