[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/include/llvm/Analysis/TypeMetadataUtils.h:80
+
+// Finds the same "relative pointer" pattern as described above, where the
+// target is `F`, and replaces the entire pattern with a constant zero.
----------------
fhahn wrote:
> shouldn't this be `///`?
Fixed (and also fixed for getPointerAtOffset).


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:204
+    if (auto *PtrExpr = dyn_cast<ConstantExpr>(U)) {
+      if (PtrExpr->getOpcode() == Instruction::PtrToInt) {
+        for (auto *PtrToIntUser : PtrExpr->users()) {
----------------
fhahn wrote:
> it might be worth turning some of those checks into early continues instead, to reduce the nesting level.
Yes. Much better now :)


================
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) {
----------------
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...


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

https://reviews.llvm.org/D109114