[PATCH] D105710: [OpaquePointers][ThreadSanitizer] Cleanup calls to PointerType::getElementType()

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D105710#2867842 <https://reviews.llvm.org/D105710#2867842>, @nikic wrote:

> In D105710#2867611 <https://reviews.llvm.org/D105710#2867611>, @dblaikie wrote:
>
>> In D105710#2867596 <https://reviews.llvm.org/D105710#2867596>, @aeubanks wrote:
>>
>>> Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to `PointerType::getElementType()`/`Type::getPointerElementType()`. The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.
>>
>> I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.
>
> My 2c here is that we mainly need to test cases where we're doing something non-obvious for opaque pointers. E.g. D105398 <https://reviews.llvm.org/D105398> uses a different code path for opaque pointers, so that's worth testing. Here we use the same code and only adjust the APIs used.

But we did so for a reason - to enable some functionality that wasn't enabled before - and we haven't tested that we successfully did what we intended to do.

> Main problem with testing these is that making the code change is easy without any domain knowledge about the pass, while writing tests for it requires you to actually figure out what it is supposed to be doing and how you can trigger the relevant code paths.

Yeah, it's not cheap, no doubt. My usual approach/suggestion would be to put an "assert(false)" in the code being changed, run check-llvm and see which tests fail, pick a fairly simple one and try the same test with opaque pointers.

Alternatively, maybe starting from the tests rather than the code would be good at this point - running chunks of the test suite with opaque pointers forced on, seeing what fails and fixing them - then it's easy to know what testing to add because it started with a failing test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105710