[PATCH] D107645: [GlobalDCE] Prepare VFE for Swift vtables (handle relative pointers and non-nfunc entries)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:158
   }
+  if (auto *CI = dyn_cast<ConstantInt>(I)) {
+    if (Offset == 0 && CI->getZExtValue() == 0) {
----------------
I think it would be good at least to have the layout documented somewhere. Not sure where the right place would be, but I think we should have at least a comment here, explaining what the expected valid layout is.

@pcc, @tejohnson any ideas?


================
Comment at: llvm/lib/Analysis/TypeMetadataUtils.cpp:168
+      case Instruction::Sub:
+        return getPointerAtOffset(cast<Constant>(C->getOperand(0)), Offset, M);
+      default:
----------------
The `sub` case here in particular needs some explanation I think. Is there anything we can verify about the layout? E.g. here the second operand always needs to be the vtable global, right?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:138
+      // Scan the !type metadata on the vtable and also ignore those deps that
+      // have an offset in one of the !type entries.
+      if (auto VTable = dyn_cast<GlobalVariable>(GVU)) {
----------------
Could you also explain *why* those dependencies can be safely ignored?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:141
+        SmallVector<MDNode *, 2> Types;
+        VTable->getMetadata(LLVMContext::MD_type, Types);
+        if (!VTable->isDeclaration() && !Types.empty()) {
----------------
Does this extra code also apply to the non-swift case? If so, could you add a non-swift related test case? 

Also, could this part split off from the extension to `getPointerAtOffset` or are both needed for any transformation to happen in the swift case?


================
Comment at: llvm/test/Transforms/GlobalDCE/virtual-functions-swift.ll:1
+; RUN: opt < %s -globaldce -S
+; RUN: opt < %s -globaldce -S | FileCheck %s
----------------
Can this simplify be removed? It is run again below and the output is piped to FileCheck.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107645