[PATCH] D109114: [GlobalDCE] In VFE, replace the whole 'sub' expression of unused relative-pointer-based vtable slots

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek added inline comments.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:206
+        for (auto *PtrToIntUser : PtrExpr->users()) {
+          if (auto *SubExpr = dyn_cast<ConstantExpr>(PtrExpr->user_back())) {
+            if (SubExpr->getOpcode() == Instruction::Sub) {
----------------
kubamracek wrote:
> fhahn wrote:
> > oh and just to double check, the assert was removed intentionally?
> Yes, I dropped it intentionally, and replaced it with a loop over all users to be more conservative.
> 
> Actually, I did end up seeing the assert fail (!) in my testing on a large codebase (compiling the Swift stdlib with VFE on), but I couldn't figure out how to isolate the exact IR that could reproduce the problem. I guess I should try harder and add it as a lit testcase...
Added "GlobalDCE/call-with-ptrtoint.ll" that triggers the assert -- in this case, we end up with a dead function that has a ptrtoint user, which itself has zero users (!), which I assume is an expected artifact of using GlobalDCE dropping all bodies of dead functions upfront. If unexpected, then it sounds like a separate bug.

In either case, I think dropping the assert and just iterating over all users of PtrExpr is the right approach here. It also makes this helper function more robust (as it doesn't assume the assert to hold).


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

https://reviews.llvm.org/D109114