[PATCH] D133307: [Assignment Tracking][14/*] Account for assignment tracking in instcombine

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:168-169
   New->setUsedWithInAlloca(AI.isUsedWithInAlloca());
+  New->setMetadata(LLVMContext::MD_DIAssignID,
+                   AI.getMetadata(LLVMContext::MD_DIAssignID));
 
----------------
Orlando wrote:
> Orlando wrote:
> > jmorse wrote:
> > > Note that D133118 fiddles with this area, as the call below to replaceInstUsesWith doesn't update debug users. With D133118 landed, I think this code is fine.
> > I'll double-check it looks right when rebasing before landing? I think it'd be okay either way (see related reply in test `alloca-bitcast.ll`).
> The test that exercises this particular modification, `alloca-bitcast.ll`, doesn't work when updated to use opaque pointers because the bitcast is noop and gets removed. I spent a little time seeing if the typed pointer codepath could be removed, but unfortunately a bunch of tests that haven't migrated to opaque pointers yet hit it.
> 
> https://llvm.org/docs/OpaquePointers.html#version-support claims that in LLVM 15.0 "Typed pointers are still available, but only supported on a best-effort basis and may be untested", so I feel that deleting the test (rather than adding a typed pointer test) is reasonable. Especially given it's a one-liner and quite "obvious". Are you okay with that @jmorse?
Hmmmmmmm. It feels bad to lose coverage of something we have coverage of -- if what you say is true, that the bitcast is deleted as a noop, then presumably someone someday is going to delete this function right? (`PromoteCastOfAllocation`). Can we have the test operate in typed-pointer mode for the forseeable future, with a comment indicating that someone can delete it when they delete this function?


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

https://reviews.llvm.org/D133307