[PATCH] D84999: dynamic_cast optimization in LTO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

@pcc since some of the questions below related to type metadata and type intrinsics he added for WPD, and he's probably a good person to resolve some of the questions regarding where to do the CXX ABI handling.

Sorry for the slow response. I'd like to resolve where and how best to recognize the CXX ABI related issues. I really think that belongs in clang and not llvm. But I suspect you don't actually need a lot of that. For example, we can go by which global vars have type metadata to identify vtables. See for example the code that builds the ThinLTO summary info for whole program devirtualization that makes the assumption about type metadata being attached to vtable defs in ModuleSummaryAnalysis.cpp (see call to computeVTableFuncs).

Perhaps all you need to do is to add some sort of llvm.type.test/llvm.assume intrinsics for __dynamic_cast calls in clang. E.g. similar to what is inserted for vtable loads under -fwhole-program-vtables for WPD. That will allow you to associate the dynamic cast with the right type metadata and vtables. E.g. something like:

  %p = call i1 @llvm.type.test({ i8*, i8* }* @_ZTI1B, metadata !"_ZTS1B")
  call void @llvm.assume(i1 %p)

>From the type info name you can get to the vtable via the type metadata.

I'm not sure whether this is legal usage of the @llvm.type.test, but hopefully @pcc can comment or give other ideas/thoughts.

I haven't had a chance to go through the guts of the IPO optimization yet, but will try to take a look through that soon. I would imagine some of the implementation will change depending on the outcome of the above questions.



================
Comment at: llvm/include/llvm/Support/CXXABI.h:25
+  switch (TT.getArch()) {
+  // all PowerPC targets use Itanium C++ ABI
+  case Triple::ppc:
----------------
Why is it only looking for PowerPC? I.e. it isn't recognizing x86_64 and a number of other things that are Itanium. See also isItaniumFamily() in clang/include/clang/Basic/TargetCXXABI.h.

I do still think that this is putting things in llvm that belong in clang, see my other comment about that.


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

https://reviews.llvm.org/D84999