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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update! Could you update the patch description? it looks empty at the moment.



================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:427
+      // avoid leaving a weird sub(0, symbol) expression behind.
+      for (auto *U : F->users()) {
+        if (auto *PtrExpr = dyn_cast<ConstantExpr>(U)) {
----------------
Would it be possible to either only iterate over all users if we know that there are relative pointers or iterate over all uses once and either replace the sub with zero or the use with nullptr?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:429
+        if (auto *PtrExpr = dyn_cast<ConstantExpr>(U)) {
+          if (PtrExpr->getOpcode() == Instruction::PtrToInt) {
+            assert(PtrExpr->hasOneUser());
----------------
Can the code here (matching ptrtoint/sub) be unified with the code that matches it in `getPointerAtOffset`, so those patterns won't diverge?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:430
+          if (PtrExpr->getOpcode() == Instruction::PtrToInt) {
+            assert(PtrExpr->hasOneUser());
+            if (auto *SubExpr = dyn_cast<ConstantExpr>(PtrExpr->user_back())) {
----------------
the assert here seems dangerous, as nothing guards against `PtrExpr` having multiple users, right? PtrExpr having multiple users is valid IR, so we should probably handle this gracefully.


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

https://reviews.llvm.org/D109114