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

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 30 08:37:29 PDT 2021


spatel added a comment.

In D107157#2916637 <https://reviews.llvm.org/D107157#2916637>, @david-arm wrote:

> 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!

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107157



More information about the llvm-commits mailing list