[PATCH] D108741: [GlobalDCE] Handle non-vfunc entries in vtables during VFE

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D108741#2998705 <https://reviews.llvm.org/D108741#2998705>, @tejohnson wrote:

> In D108741#2997844 <https://reviews.llvm.org/D108741#2997844>, @fhahn wrote:
>
>>> As part of codesize optimization work for Swift, we'd like to add Virtual Function Elimination to Swift, very similarly to how GlobalDCE supports C++ VFE.
>>
>> I think it would be good to drop this part from the description, as the change is not really related to swift; it just changes the code to ignore entries at slots without `!type`.
>>
>> It seems sensible to me to ignore entries without corresponding `!type` metadata  and this should be in line with the specification in langref. @pcc, @tejohnson Are there any issues you could think of?
>
> Is this specified in the langref? I looked through the type metadata documentation but don't see it mentioned there that all vfuncs will have type metadata. Looks like that was added a bit later than the original type metadata, in D47567 <https://reviews.llvm.org/D47567>. I think what is specified in langref is that it has type metadata for the address point. Probably the documentation needs updating. But from what I can see, it looks like this should always be emitted now. @pcc ?

Thanks for checking! I think the update to the langref in the latest version should spell out things clearly.



================
Comment at: llvm/docs/TypeMetadata.rst:288
+
+1. All virtual function pointers in the vtable must have a matching ``!type``
+attachment. Function pointers without a ``!type`` attachment are not
----------------
Not sure about the rst syntax here. Does this render correctly as list?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:149
     }
+
     GVDependencies[GVU].insert(&GV);
----------------
nit: unrelated change


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:184
     Types.clear();
+    VFuncs.clear();
     GV.getMetadata(LLVMContext::MD_type, Types);
----------------
can we instead just declare VFuncs in the loop?


================
Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:204
+                                           *GV.getParent(), &GV);
+      C = C ? C->stripPointerCasts() : nullptr;
+      if (auto VFunc = dyn_cast_or_null<GlobalValue>(C)) {
----------------
Does C being `nullptr` here indicate that the type id is invalid?


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

https://reviews.llvm.org/D108741