The revision title and summary contradict each other. The title says to allow something, but the summary says that already works; then the summary says CHECK-DAG can't be first, while the check-dag.txt test clearly already has CHECK-DAG as the first directive.
Please fix up the title and summary so that they correctly describe what you are trying to accomplish. This will help reviewers to make sure the patch does what you want.

Thanks! I'm not surprised I didn't get it completely right; it was such a maze of twisty passages all different.

LGTM although I do wonder at the usefulness of the wildcards in the implicit-not checks.

LGTM

I'm happy.

Nice work on the prefix tests. One comment nit and LGTM.

A couple of comment phrasing nits.

Sorry for not noticing this sooner. The TL;DR is, the current patch does not affect PS4, so go ahead; but see the long version for other considerations.

In D54769#1317228, @jdenny wrote:

Another problem would be if lit substitutions make FileCheck commands or their prefixes unrecognizable. I haven't looked for that usage yet.

A few more comments.

Thanks for the ping. The code change looks straightforward, but a handful of questions on the tests.

In D56297#1346801, @andrewrk wrote:

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

Changes to CommandLine.* should be tested in llvm/unittests/Support/CommandLineTest.cpp, as a rule. We don't want to rely on FileCheck tests to verify code in Support.

I am willing to review this; however it is a very large patch, which will take time to go through. Also, after tomorrow, I am on holiday until 3 January.

This will permit -DVALUE= (with no value) is that a supported usage?
It also permits -D=10 which I daresay doesn't mean anything.

A couple small things. If Jonas is happy, I'm happy.

In D55281#1334789, @dblaikie wrote:

In D55281#1334783, @probinson wrote:

In D55281#1334706, @dblaikie wrote:

I'm just still not sure the extra cl::opt is worth it, though - are you? Is the extra coverage significant/important?

I'd rather see a test break locally (because it's target-agnostic and uses the flag) than wait for some NVPTX-target bot to break after I commit.

Do you trim the target set you build/dev with locally? Does that save much time - I build with the default, all the (non-experimental) targets enabled, so NVPTX failures do happen locally.

In D55281#1334706, @dblaikie wrote:

I'm just still not sure the extra cl::opt is worth it, though - are you? Is the extra coverage significant/important?

I missed that filcab had said ok internally. Go ahead Pierre.

In D55281#1333802, @dblaikie wrote:

Still a bit curious about the other discussion about flags, etc.

In D55281#1333552, @dblaikie wrote:

@aprantl @probinson - you folks haev any feelings about this? Looks like these flags are essentially unused (except for testing, which could equally be accomplished by tests that specify an NVPTX triple, I think (except on builds that aren't building an NVPTX backend)) & are a collection of features that are all only needed on NVPTX. I'd lean towards removing them all & gating this on NVPTX. Or on one flag rather than several fine-grained ones, if/when someone starts playing with a ptx assembler that can cope with these features.

But yeah, no big deal I guess - leads to a bit of confusion about who these flags are meant for, under what conditions some of these features might be enabled but not others, etc.

LGTM but I will defer to @filcab as he is more familiar with the area.

Much better, thanks! LGTM

LGTM

In D55721#1331847, @dblaikie wrote:

In D55721#1331812, @probinson wrote:

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

Using [C] (C = count) for both the count-only and upper-bound-only cases suggests that we're being descriptive rather than carefully reflecting the attributes that are in the subrange DIE, which is fine. So, if an explicit LB matches the language default, then the display should also be [C] (where C = count if we have a count, or C = U + (1 - L) if we have upper_bound).

The only change you're suggesting is to detect when an explicit LB matches the language default, and treating that case the same as if the LB were not specified (in terms of how it's printed out)?

Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.

In D53897#1331582, @jdenny wrote:

Yes, a follow-up would be easier. I'll just commit something. If you prefer a phab review first, that's absolutely fine for me, but let me know.

In D53899#1328955, @jdenny wrote:

Remove MatchTypeCount, which is unused as of this patch.

LGTM

The terminology around "possible" "expected" and "final" matches probably warrants a few lines of commentary somewhere, unless I missed it in an earlier patch. There are getting to be enough cases that the casual reader (ahem) can't just Zen their way through them all.
Can be done as a follow-up if you prefer.
One comment nit and LGTM.

LGTM although I think John should have a chance to get in a last word.

In D36993#1328536, @dblaikie wrote:

FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.

These get to be pretty mechanical after a while, don't they? I was wondering how hard it would be for someone else, next time somebody wants to add a new directive kind, but it seems like not really a big deal and that's a good thing.
Again a comment needs fixing but otherwise LGTM.
I'll try to get to the rest tomorrow.

Comment issue, otherwise LGTM.

One small comment fix needed, otherwise LGTM.

A couple of nits I didn't notice until looking at the next patch.

Abandoning dead patch. This wound up being done a different way.

I like having the extra explanations at the end of the tilde lines, that's a great idea.
One unnecessary include left, and LGTM.

Does the verifier bail out after the first failure? If not, you can combine the failure tests all into one test.

In D55272#1323317, @vsk wrote:

LGTM. @CarlosAlbertoEnciso, any thoughts on this?

Easy enough to accommodate NVPTX, if you use DWARF version to pick constant size versus label diff. I don't remember what it takes to work out what the target is at this point, but version is obviously easy.

You're using !~~~ to flag: (a) CHECK-NOT that has a match, (b) CHECK-NEXT that isn't in the right place, (c) a discarded DAG match.
The first two are errors (which would be reported regardless of annotations) but the third is not. So, I'm not sure why it should have the same kind of annotation.

In D54769#1317118, @dblaikie wrote:

In D54769#1317107, @jdenny wrote:

I'm not sure which tests would be "simple", but any test that generates the FileCheck check file dynamically would certainly not work with such a script.

I don't think there are any of those - at least I rather hope not.

I have no answer for the address-space questions. I'd say ask on llvm-dev, as it's worth asking in a more visible forum.
Assuming we are able to say that nullptr is zero for address space zero, the patch seems fine (and modifying an existing test is perfectly fine).

It looks like this patch would fail to reject cases such as:

.file 1 "a.c"
.file 1 "b.c"

and if that's true, it's a problem.

+ Matt Davis who wrote the test
Sorry for the delay; feel free to add a 'ping' reply every week or so when you get no action.
Seems right to me but I think Matt would be a better choice to review.

The indentation in the test file looks a little funny; please make sure there are no hard tabs in the file. Aside from that LGTM.

In D55091#1314919, @vsk wrote:

Your DWARF analysis tool presumably needs to support diffing and output serialization. So would a comprehensive code size analysis tool. There's no reason why these two tools should have different diffing / serialization options. The latter shouldn't live within a debug info specific tool. However, there's no fundamental issue with adding DWARF to the long list of formats llvm-objdump already understands. Technically, it's just a matter of linking in llvm's DWARF libraries.

LGTM aside from a grump about the test, which is totally up to you.

Apologies for the delay. I haven't been ignoring this series; I was having internal qualms about the amount of effort to produce extensive annotations, and the value they might provide. But I've come down in favor of doing it.

When building the FileCheck binary with debug info, this patch makes the build artifacts ~1kb smaller.

In D54769#1309251, @SjoerdMeijer wrote:

But I still disagree with this one:

And if the input was this:

RUN: llc -mtriple=arm-linux-gnueabi %s -o - | FileCheck %s -check-prefix=FOO
define void @FOO() {
entry:
; FOO-LABEL: FOO:
; DOO:             mov  pc, lr
; FOO:             ret void
}

there is no reasonable way to flag the DOO line as a mistake. In fact, typos in the prefix are a tactic I've often used to "comment out" directives, for one reason or another, and I would not want that tactic to start triggering warnings.

I think this is the prime example where I would like to see and get a diagnostic. In this example, I think there are 2 bugs: 1 in the test, and 1 in FileCheck. There are probably many ways to temporarily comment out things and this is a "dangerous" one. Because allowing this relaxed FileCheck behaviour it's too easy to shout ourselves in the foot, and that's why we have so many test issues. The fact that we have so many test issues, shows that code review isn't always catching this. But I don't think code review should not really have to deal with this. We have a tool for this: FileCheck should do its checking, and do it properly. Besides 'commenting out' checks in this way, the other cause for this if you first had --check-prefixes=FOO,DOO, then some test refactoring happens, DOO is dropped from the --check-prefixes line, and the test becomes half useless, half correct.

In D54755#1313679, @dexonsmith wrote:

In D54755#1313098, @probinson wrote:

In D54755#1313064, @probinson wrote:

In D54755#1312971, @aprantl wrote:

While I was updating some internal testcases I wondered: why is DIFlagPrototype not a DISPFlag? Does this flag make sense on something other than functions?

Because I was not interested in moving existing flags from the old word to the new word in the first round. There are others that probably apply only to subprograms:
NoReturn, MainSubprogram; probably Thunk, All CallsDescribed; maybe Trivial, Explicit.
Really we should do some kind of audit, and do a bulk move in one go so we can minimize the bitcode-upgrade pain.

One additional problem is that the existing flags are exposed as part of the public API, i.e. to front-ends, so we'll need to work out how to avoid Bad Stuff(tm) if we repurpose any existing flag bits.

Another option here is not to merge them at all in bitcode (I didn't consider this before). You can add an abbreviation for a fixed-width field of 1 bit each:
https://llvm.org/docs/BitCodeFormat.html#fixed-width-value

Are those hard tabs in the test .s file? many editors do that automatically in assembler source, but the LLVM project doesn't want hard tabs.

In D54755#1313064, @probinson wrote:

In D54755#1312971, @aprantl wrote:

While I was updating some internal testcases I wondered: why is DIFlagPrototype not a DISPFlag? Does this flag make sense on something other than functions?

Because I was not interested in moving existing flags from the old word to the new word in the first round. There are others that probably apply only to subprograms:
NoReturn, MainSubprogram; probably Thunk, All CallsDescribed; maybe Trivial, Explicit.
Really we should do some kind of audit, and do a bulk move in one go so we can minimize the bitcode-upgrade pain.

In D54755#1311897, @dexonsmith wrote:

Nice!

In D54755#1312971, @aprantl wrote:

While I was updating some internal testcases I wondered: why is DIFlagPrototype not a DISPFlag? Does this flag make sense on something other than functions?

My patches have landed, and look like they'll stick; please rebase and use the DISubprogram specific flags for the new Fortran flags.

In D54756#1311679, @aprantl wrote:

Everybody with out-of-tree tests will be excited ;-)

In D54755#1311498, @aprantl wrote:

I think it would be nice to have one small explicit "DISubprogram version 4" .bc upgrade test, otherwise this LGTM now. Thanks!

Ping. @aprantl approved the dependent patch D54755.

I am curious how debug info for common blocks plays with LTO. I think we would not want different common-block descriptions to be de-duplicated just because the global name was the same.

In D54043#1310486, @schweitz wrote:

Thanks, Paul.

Or this could go in first and supply additional tests for your new bit format. :)

For what it's worth, the overloaded bit-encoding follows the precedent already established by LLVMDIFlagIndirectVirtualBase.

Don't emit unused fields to the bitcode. Accommodate the new variant in the reader.

In D54043#1310116, @schweitz wrote:

I believe all the comments have been addressed. Is there anything else required on our end?

In D54769#1308918, @SjoerdMeijer wrote:

And I do remember a possibly valid use case for emitting a diagnostic. I've seen quite a few tests with names like test-pr12345.ll, that were reproducers of compiler crashes, but only contained CHECK-LABELS. I called them dubious tests before (but we don't want to use that term), because these tests are happy when a functions compiles, but don't tests anything whereas they probably should. There are probably a few exceptions where only CHECK-LABELs are okay, so I think a warning along the lines of "you're only checking labels, is that what you really want?" should be fair?

In D54769#1308894, @SjoerdMeijer wrote:

So, I'm not convinced that we can have a diagnostic for your case 3 that would really be meaningful and useful.

That is really unfortunate and problematic. Apologies, but just for completeness, this was that example:

RUN: llc -mtriple=arm-linux-gnueabi %s -o - | FileCheck %s -check-prefix=FOO
define void @FOO() {
entry:
  ; FOO-LABEL: FOO:
  ; DOO:             mov  pc, lr
  ret void
}

It's really not difficult to make this mistake, and I really would like to solve this one too. My previous approach might be wrong, so I am going to look again if I can get it to reliably error in this case.

In D54769#1308872, @SjoerdMeijer wrote:

I am unsure about 3), i.e. I can't remember/am unsure if we can avoid false positives. I will look into this again, or if someone can remind me that's fine too :-). But I thought this was the motivating case for emitting a diagnostic: if a false positive cannot be easily be avoided, an opt-in warning is the best we can do at the moment. The use case would indeed be to enable this while developing new tests.

In D54769#1308690, @jdenny wrote:

In D54769#1308654, @dblaikie wrote:

You/others could checkin fixes as they find them rather than accumulating one large patch - then when it's close to done, you send out the patch to add the error to avoid regressions?

Adding -warn now makes it easier for anyone to contribute to this cleanup and doesn't hurt anyone who doesn't care about it.

In D54755#1308216, @aprantl wrote:

I'm assuming we already have plenty of DISubprograms with flags in the old format as .bc tests that cover the bitcode upgrade?

Ping.

This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.