The tests previously had lots of unnecessary CHECK lines, where
all we really need to check is the presence (or absence) of the
assume intrinsic and the correct input operands.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
I assume (pun...) that I updated this file with the complete checks while making a vectorizer change that may or may not have made it to trunk commit.
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.
So IIUC, this patch is likely to remove this file from some generally unrelated upcoming code change. But if that's the gain, is it worth the cost of making it harder for other, less expert contributors to make changes? Let me know if I got that wrong. I won't block this either way, but hopefully that explains why we currently have the full checks.
If we do remove the full checks, please remove or update the first line in the file, so we're not misleading about the script usage.
| llvm/test/Transforms/LoopVectorize/assume.ll | ||
|---|---|---|
| 0–1 | This isn't true anymore - please delete it | |
Comment Actions
Hi @spatel, you make a valid point that the update script makes it a lot easier to add tests and so on. I completely agree. The problem I found was that it took me a long time to figure out what was actually being tested here that's all. For example, in predicated_assume we're actually testing that we *haven't* added the assume intrinsic, so I thought a CHECK-NOT line made it very obvious that's all.
In general, and this is nothing to do with your tests (which are good tests by the way!), I sometimes find with all the CHECK lines for vectoriser tests is that there is a high chance that your patch changed something that actually has nothing to do with what the test is testing, and it can also consume quite a bit of time trying to understand if you've genuinely broken something or not. I do see though that there are pros and cons either way!
If you prefer to leave the tests as they are that's fine by me. :)
Comment Actions
Yep - there's always going to be that trade-off.
If it's possible, can we reduce the IR and/or RUN line params further, so we have less test filler before getting to the main idea? I know that's not as easy for LV as less complex passes, so no worries if not.
If you prefer to leave the tests as they are that's fine by me. :)
I'm fine either way, so I'll leave it up to people who spend more time hacking on LV than me.
Comment Actions
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 :)
Comment Actions
+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.
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.
This isn't true anymore - please delete it