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

Fri Jul 30 08:58:39 PDT 2021

fhahn added a comment.

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

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

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 :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107157