[PATCH] D108741: [IR][GlobalDCE] Add ability to mark vtable methods as eligible for VFE and avoid eliminating non-eligible vfunc in VFE in GlobalDCE

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek updated this revision to Diff 380140.
kubamracek added a comment.

I have talked with @rjmccall offline, and I'm going to attempt to summarize his thought. @rjmccall, please correct me where I' wrong :)

- What feels weird about the current patch (adding the "vfe_eligible" constant) is the fact that in the IR we already have 2 "things" (!type and !vcall_visibility) that are needed for VFE correctness and effectivity, and we're now just adding a third one.

- It would be better to see some sort of consolidation so that *all* the information that VFE needs to know about a vtable is contained in a single "entity", perhaps a new piece of metadata, with a format that is extended to support all the features everyone (Clang, Swift) needs.

- There is at least one interesting opportunity that we should consider and make it expressible in IR: In C++, having a subclass with a more restricted vcall visibility than the base class should allow VFE to optimize a part of the vtable with more precise information. However, that's currently not expressible because there's only one !vcall_visibility on the global.

- The original problem I was trying to solve here can be generalized and tackled together with this C++ vtable expressivity issue: We could extend !vcall_visibility to allow specifying a "range" of offsets in the vtable that the visibility marker applies to, and also allow specifying multiple !vcall_visibility attributes on a single vtable. While this does not achieve the IR design "consolidation", we could perhaps consider that as a intermediate minimally-invasive step in the right direction.

Based on that, I would like to actually propose continuing the IR design discussion on llvm-dev (I'm going to start an email thread on that), while at the same time I'd like to pursue the change of extending !vcall_visibility (add "offset range" to it) to address the immediate problem I need solving for VFE on Swift code. Practically that means !vcall_visibility would accept two formats:

1. `!vcall_visibility !{i64 2}` -- visibility only
2. `!vcall_visibility !{i64 2, i64 0, i64 32}` -- visibility, range start, range end

The first form means there is an implicit range that covers the entire vtable.

@pcc, would you be okay with such a direction instead of the suggested vfe_eligible constant? I have changed the diff with an implementation of that.


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

https://reviews.llvm.org/D108741

Files:
  llvm/docs/TypeMetadata.rst
  llvm/include/llvm/IR/GlobalObject.h
  llvm/include/llvm/Transforms/IPO/GlobalDCE.h
  llvm/lib/IR/Metadata.cpp
  llvm/lib/Transforms/IPO/GlobalDCE.cpp
  llvm/test/Transforms/GlobalDCE/virtual-functions-non-vfunc-entries.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108741.380140.patch
Type: text/x-patch
Size: 10661 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20211016/dc4049e2/attachment-0001.bin>