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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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.

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.



================
Comment at: llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp:617
   Value *Addr = IsWrite ? cast<StoreInst>(II.Inst)->getPointerOperand()
                         : cast<LoadInst>(II.Inst)->getPointerOperand();
+  Type *OrigTy = getLoadStoreType(II.Inst);
----------------
Could you getLoadStorePointerOperand() here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105710