[PATCH] D108741: [IR][GlobalDCE] Extend !vcall_visibility to be able to specify an offset range that it applies to

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/docs/TypeMetadata.rst:296
+range is missing, the meaning is the same as a range covering the entire vtable.
+Any part of the vtable that is not covered by the specified range is not
+eligible for elimination of virtual functions.
----------------
might be good to spell out that the range is a half-open (< end)


================
Comment at: llvm/include/llvm/IR/GlobalObject.h:141
   VCallVisibility getVCallVisibility() const;
+  std::tuple<uint64_t, uint64_t> getVTableOffsetRange() const;
 
----------------
nit: could just use `std::pair`?


================
Comment at: llvm/include/llvm/Transforms/IPO/GlobalDCE.h:52
+  /// specified in !vcall_visibility).
+  DenseMap<GlobalValue *, SmallPtrSet<GlobalValue *, 8>> VFESafeVTablesAndFns;
 
----------------
nit: I think it would be good to include some of the original wording, saying that contains global variables which are vtables for which we have enough information to safely do VFE, together with the functions in the vtable for which VFE is safe.


================
Comment at: llvm/lib/IR/Metadata.cpp:1546
+          cast<ConstantInt>(
+              cast<ConstantAsMetadata>(MD->getOperand(2))->getValue())
+              ->getZExtValue();
----------------
it would be good to have a the IR verifier check that the second and third operands are integer constants < UINT64_MAX.


================
Comment at: llvm/lib/IR/Metadata.cpp:1551
+  }
+  return std::tuple<uint64_t, uint64_t>(0, UINT64_MAX);
+}
----------------
nit: consider using the more C++ like `std::numeric_limits<uint64_t>::max()`


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:132
+    // range specified in !vcall_visibility, and we have
     // complete information about all virtual call sites which could call
     // though this vtable, then skip it, because the call site information will
----------------
nit: re-flow comment.


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:135
     // be more precise.
-    if (VFESafeVTables.count(GVU) && isa<Function>(&GV)) {
+    if (isa<Function>(&GV) && VFESafeVTablesAndFns[GVU].contains(&GV)) {
       LLVM_DEBUG(dbgs() << "Ignoring dep " << GVU->getName() << " -> "
----------------
nit: should we use `find` to look up `GVU` here to avoid inserting new entries in the map?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:169
+    if (auto *F = dyn_cast<Function>(GV)) {
+      if (RangeStart <= BaseOffset && BaseOffset < RangeEnd) {
+        VFuncs->insert(F);
----------------
nit: I think usually `{}` are omitted for single-line bodies.


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:184
+      unsigned Op = SL->getElementContainingOffset(Offset);
+      FindVirtualFunctionsInVTable(M, cast<Constant>(S->getOperand(Op)),
+                                   RangeStart, RangeEnd, VFuncs,
----------------
could we use `getPointerAtOffset` here and below instead of recursing further? That way we would only add function pointers that fit the supported Vtable scheme and we should be able to avoid handling arbitrary constants by iterating over their operands, which might lead us to discover function pointers that are not valid VTable entries.


================
Comment at: llvm/test/Transforms/GlobalDCE/virtual-functions-non-vfunc-entries.ll:5
+
+declare { i8*, i1 } @llvm.type.checked.load(i8*, i32, metadata)
+
----------------
It might be good to also have a test with a relative pointer vtable


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

https://reviews.llvm.org/D108741