[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 a comment.

IMO this approach is preferable to adding a new type of constant, because it is much more lightweight. The use-case also seems to fit somewhat with the original goals of `vcall_visibility`, which we already use to determine of the *whole* vtable is safe for VFE. Extending it to also specify which functions are safe for VFE makes sense to me.

It restricts the eligible functions to a single range, which is not as powerful as the new constant approach. Just to double check, will this be sufficient for your use cases?



================
Comment at: llvm/include/llvm/Transforms/IPO/GlobalDCE.h:52
+  /// specified in !vcall_visibility).
+  DenseMap<GlobalVariable *, SmallPtrSet<GlobalValue *, 8>> VFuncMap;
+
----------------
Could this subsume `VFESafeVTables`? IIUC `VFuncMap` should only contain VTables that are safe for VFE together with the set of functions for which VFE is safe. Maybe also use a more descriptive name, like `VFESafeVTablesAndFns`.


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:126
+  auto *VTable = dyn_cast<GlobalVariable>(VTableVal);
+  assert(VTable);
+
----------------
nit: you can just use `cast` instead of `dyn_cast` and get rid of  the assert; `cast` will assert if the result is null.


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:128
+
+  if (VFuncMap[VTable].contains(VPtr))
+    // Have a match in VFuncMap, i.e. have VPtr is within the range specified in
----------------
nit: just `return VFuncMap[VTable].contains(VPtr)`?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:259
+            M, GV.getInitializer(),
+            [&Range, &VFuncs](GlobalValue *VPtr, uint64_t Offset) {
+              if (std::get<0>(Range) <= Offset && Offset < std::get<1>(Range)) {
----------------
Is this callback here really necessary or could it be in `FindVirtualFunctionsInVTable`? It seems to make it harder to see what's going on when reading  `FindVirtualFunctionsInVTable` in isolation.


================
Comment at: llvm/test/Transforms/GlobalDCE/virtual-functions-non-vfunc-entries.ll:33
+  i8* bitcast (void ()* @regular_non_virtual_funcB to i8*)
+]}, align 8, !type !0, !type !1, !vcall_visibility !{i64 2, i64 0, i64 16}
+
----------------
we should also have a test with a start offset different than 0.


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

https://reviews.llvm.org/D108741