[PATCH] D84999: dynamic_cast optimization in LTO

[wael yehia via Phabricator via llvm-commits]

w2yehia added a comment.

Teresa and Steven, thanks for the comments.



================
Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1474
 
+static void ComputeExtraHint(llvm::CallInst *Call, llvm::Value *HintArg,
+                             const CXXRecordDecl *Src, const CXXRecordDecl *Dst)
----------------
tejohnson wrote:
> Needs a description as to what this function is doing. Also a much more specific name. "ExtraHint" doesn't say much.
Does `SetNonPrivateBaseAttr` sound better?
By the way, the Itanium ABI uses the term "hint" for the 4th parameter, so I thought this would be a natural extension, but definitely can use an accurate name for what we're doing here.


================
Comment at: llvm/lib/Transforms/IPO/DynamicCastOpt.cpp:15
+// types, and B is a public base class of T, then an equivalent expression is:
+//    typeid(T) == typeid(*expr)
+//
----------------
steven_wu wrote:
> I think this needs to be `operator==` for `std::typeinfo` for this to work for non-unique RTTI. The current pass seems to lower this into `icmp` which might cause miscompile. 
true... I wasn't sure how to achieve that.
The operator== function is defined in the <typeinfo> header, and not in the std library for us to generate a call to.
Ideally clang would tell us whether the RTTI is unique, but I don't think it has access to that info since it's determined in the typeinfo header.
Any suggestions?


================
Comment at: llvm/lib/Transforms/IPO/DynamicCastOpt.cpp:131
+// common strings
+StringRef TIPrefix("_ZTI");     // _ZTI1B = typeinfo for B
+StringRef VTablePrefix("_ZTV"); // _ZTV1B = vtable for B
----------------
tejohnson wrote:
> Not a language expert, but I don't think it is safe or a good idea for LLVM to be making assumptions about the mangling (which can vary e.g. Microsoft is quite different). You probably need to have clang encode additional info in either metadata or an llvm intrinsic. Perhaps you can leverage the type checks normally inserted for vtable loads for WPD under -fwhole-program-vtables.
a very valid cocern;
I'm doing some refactoring to hide the ABIism behind an interface in this pass, and have that interface support Itanium ABI for now.
Will update this patch shortly.


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

https://reviews.llvm.org/D84999