In D85004#2188062, @danielkiss wrote:

Added here as I see:
https://github.com/llvm/llvm-project/blob/19bc9ea480b60b607a3e303f20c7a3a2ea553369/libcxxabi/test/CMakeLists.txt#L57

Isn't it missing DEPENDS cxxabi the same way this was missing DEPENDS unwind?

Mind doing the same thing for libcxxabi? I think it has the same problem.

LGTM, thank you!

@njames93 I believe this change broke one of the buildbots (verified by reverting it locally). Mind taking a look? From a quick glance, I can't tell where the functional change is here.

736423af53d707e097a174c3a91b75132b8dc6b1

In D84327#2173284, @lebedev.ri wrote:

LG to me, but probably wait a bit for @jroelofs.

4dc3014c51fda2a3189318c4ae54c4da9cfc6a0e

In D83818#2173227, @SjoerdMeijer wrote:

Optional: since they are really small tests, you can try if it is convenient to have them all in one file, but separate is fine too I think.

In D84450#2173234, @fhahn wrote:

Thanks for sharing. I did not manage to get it to crash with the patches removed though :(

@MaskRay I believe this broke one of the buildbots when it landed. Mind taking a look?

Kinda verbose, but it works. This would be so much easier if we had a way to dump mir from llvm-mc.

rm subsumed test

Add mir tests for each of the affected instructions.

Add a test for the changes for fjcvtzs.

In D84071#2169764, @jroelofs wrote:

In D84071#2169440, @fhahn wrote:

Do you happen to still have the test case for this? If it is reasonably small, would it be possible to add to the tests?

I have the IR, but the assert doesn't repro if I run it through the pass via opt. I'll see if I can reduce it a bit so we at least have a test where the PM reports the change.

In D84071#2169440, @fhahn wrote:

Do you happen to still have the test case for this? If it is reasonably small, would it be possible to add to the tests?

add test case, clang-format

@SjoerdMeijer mind double-checking me on this?

Good call. This is definitely a better place for that to live.

• Add back the "also matches previous" part of the diagnostics.
• Fix formatting that the linter complains about.

In D84207#2164611, @MaskRay wrote:

LGTM. Thanks for fixing it!

... on systems where wait() isn't one of the declarations transitively included via unistd.h.

Hope you can just be more specific about this - on which particular system wait() isn't transitively included via unistd.h
It conveys more information and helps posterity when they are reading the log.

review feedback

I found a way to repro it, and this patch doesn't have an effect on it.

In D83449#2140761, @varungandhi-apple wrote:

I don't understand the clang-tidy/clang-format complaints. I've formatted the code based on how the surrounding code is formatted.

LGTM

In D79963#2062377, @jhenderson wrote:

There's probably not much risk of false negatives with that, but I don't know for sure.

In D79963#2055419, @jroelofs wrote:

In D79963#2054177, @jhenderson wrote:

This will still cause a small number of false positives (e.g. the three llvm-readobj tests would all generate them, I believe), but it would be easier to reorganise the comment to not trip over it without making it look "ugly" and inconsistent with other comments elsewhere. I guess there might be some false negatives, but I doubt there'd be all that many at all?

I like that too. I'll prototype this, and see if I can get some concrete numbers on the change in diagnosed cases. Also promise to summarize on llvm-dev (haven't sparked that discussion back up yet since I've been quite busy, but I definitely see the importance / value of it).

1e06b169be3e59799b8dcaf16d1d03bd4c12da42

In D79963#2054177, @jhenderson wrote:

The error message improvement will help somewhat, but it doesn't solve the initial problem of "I wrote my comment in the same style as all the surrounding tests, it is easy to read in its current form, but not FileCheck wants me to change it to something that doesn't read as well (because it starts looking like a FileCheck/lit directive which has some meaning)."

In D79963#2038710, @jroelofs wrote:

In D79963#2038058, @jhenderson wrote:

I saw the benefit initially when the proposal was made, but now seeing the consequence in practice in the tools tests at least makes me realise I'm not a fan of the change to make FileCheck error if it sees the string somewhere else in the line. In particular, GNU and LLVM are common prefixes used to distinguish checks for different output modes, but also will often appear in comments to explain the test cases. Similar comments apply to the machine names etc. Needing to add COM is unobvious, and makes the comments messier to read, I found, as soon as I started reading.

Let’s not lose sight of the fact that this whole change is a trade off: we are attempting to trade comment succinctness/obviousness for the ability to diagnose a particularly nasty failure mode of missing colons. This comment thing may not be the right tradeoff, but given how constrained the problem space is, I suspect we will end up sacrificing a bit of intuitiveness somewhere regardless. So long as these new diagnosed-but-unintuitive cases aren’t too frequent/annoying, IMO that’s a better place to be than having room for the other not-diagnosable-and-unintuitive failure mode.

In D79963#2050521, @jhenderson wrote:

In D79963#2049161, @jroelofs wrote:

183d6af081899973f00fc24aeafcfc32de732f02

Why was this landed? There was varying degrees of dislike from more than one reviewer (myself, and @MaskRay at least). Please revert pending a conclusion to the discussion. This has hardly reached consensus.

183d6af081899973f00fc24aeafcfc32de732f02

@atanasyan there's one more case of this in llvm/test/CodeGen/Mips/ci2.ll. Looks like the constant has been de-duplicated since that test was written. Mind checking whether that's correct behavior for your target? (it does seem better, but I don't want to assume).

In D79963#2039800, @echristo wrote:

The first patch seemed fine and pretty useful. I'm not certain that this is a particularly fun idea as far as having to redo all comments etc.

In D79963#2038058, @jhenderson wrote:

I saw the benefit initially when the proposal was made, but now seeing the consequence in practice in the tools tests at least makes me realise I'm not a fan of the change to make FileCheck error if it sees the string somewhere else in the line. In particular, GNU and LLVM are common prefixes used to distinguish checks for different output modes, but also will often appear in comments to explain the test cases. Similar comments apply to the machine names etc. Needing to add COM is unobvious, and makes the comments messier to read, I found, as soon as I started reading.

Implement review feedback

In D79963#2037408, @jdenny wrote:

This looks like a move in the right direction to me, but I'm biased.