[PATCH] D107157: [NFC] Clean up tests in test/Transforms/LoopVectorize/assume.ll

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

In D107157#2916776 <https://reviews.llvm.org/D107157#2916776>, @fhahn wrote:

> Personally I think it makes sense for those tests to just check for the presence or absence of the expected assume + the instructions feeding them, as this is what the tests focus on. The scaffolding around it may change in small ways, but those changes are not really interesting for those tests :)

+1. IMO the test-update scripts are useful for smaller snippets of IR, or to help generate CHECK lines for a larger function which are a bit of a pain to to write by hand. With the CHECK lines more focussed to what the test aims to test, it's easier to understand what's being tested, and it avoids having to update/regenerate the tests for unrelated changes.

In D107157#2916521 <https://reviews.llvm.org/D107157#2916521>, @spatel wrote:

> With full checks, the process of seeing/making changes is automated and so someone who is not expert at the intent of these tests can easily submit the diffs for review (and someone more knowledgeable can pass judgement). It may create some noise/work for subsequent patches, but it also makes it highly visible when a patch is having an unintended consequence.

I see your point and don't disagree, but want to point out that there is a possible downside to this as well in that people may become complacent in trying to understand whether their change is correct and simply regenerate the CHECK lines for the test. If that's not caught by one of the reviewers, then it may go through unnoticed. Although I guess you could also argue that the quality of all patches hinge on good code review :)

>From what I read, it seems there is no objection for the approach set out in this patch, so in that case I'm happy to accept it.


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

https://reviews.llvm.org/D107157