WholeProgramDevirt: print remarks with devirtualized method names.

[krasin]

Summary

Chrome on Linux uses WholeProgramDevirt for speed ups, and it's
important to detect regressions on both sides: the toolchain,
if fewer methods get devirtualized after an update, and Chrome,
if an innocently looking change caused many hot methods become
virtual again.

The need to track devirtualized methods is not Chrome-specific,
but it's probably the only user of the pass at this time.

[kcc]

LGTM with a nit

lib/Transforms/IPO/WholeProgramDevirt.cpp

614

maybe "const std::vector &" ?

[krasin]

Add const.

[mehdi_amini]

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

615 ↗

(On Diff #66998)

For the fully devirtualized case, we do it only if F is not overridden, i.e. if every Target has the same Target.Fn. It means your loop will print "devirtualized " + Name many times without good purpose.

620 ↗

(On Diff #66998)

Avoid std::string, please use Twine instead.

835 ↗

(On Diff #66998)

This is incorrect: you'll report "devirtualized" where it may not.

[krasin]

Mehdi,

thank you for your comments. Would it be okay to send similar CLs for your review, while Peter is on vacation?

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

615 ↗

(On Diff #66998)

I observed that, and it does not really hurt (unlike the issue below). At the same time, it would be nice to print them only once. Any suggestions about the best way to do it?

620 ↗

(On Diff #66998)

Thanks, will do in a follow up CL (soon; today).

835 ↗

(On Diff #66998)

Can you please give an example? I currently don't see why would that be the case, but I have an indirect confirmation that I indeed print more than actually devirtualized.

[mehdi_amini]

In D23219#507432, @krasin wrote:

thank you for your comments. Would it be okay to send similar CLs for your review, while Peter is on vacation?

Sure.

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

615 ↗

(On Diff #66998)

Yes: remove the loop and peak only the first elt :)
The name of the function emitTargetsRemarks is also a bit generic for what it does.

835 ↗

(On Diff #66998)

This is first because tryVirtualConstProp does not perform straight devirtualization like trySingleImplDevirt, so you'll report a "devirtualized" function while only one of the call (or none, the return value is lying here...) may be actually devirtualized.

You may be able to emit more accurate information by moving this into the loop that is commented // Rewrite each call to a load from OffsetByte/OffsetBit. line 594.

[krasin]

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

615 ↗

(On Diff #66998)

Hm... I actually added some debug prints and compiled my little program that I use to play with these remarks (http://pastebin.com/yww92KZm):

remark: devirt.cc:62:23: devirtualized call
remark: devirt.cc:34:0: i=0 devirtualized A::isOfType(ObjectType) const
remark: devirt.cc:46:0: i=1 devirtualized D::isOfType(ObjectType) const
remark: devirt.cc:58:23: devirtualized call
remark: devirt.cc:34:0: i=0 devirtualized A::isOfType(ObjectType) const
remark: devirt.cc:40:0: i=1 devirtualized B::isOfType(ObjectType) const
remark: devirt.cc:51:0: i=2 devirtualized C::isOfType(ObjectType) const
remark: devirt.cc:46:0: i=3 devirtualized D::isOfType(ObjectType) const

While there're some dups, but they are from different emitTargetsRemarks invocations (as the program has two call sites), so I will need to keep a set of already reported methods to avoid them.

This is also the case where virtual const propagation takes place.

For the reference, the command line I used:

clang++ -fuse-ld=lld -o lala devirt.cc -std=c++11 -flto -fvisibility=hidden -fwhole-program-vtables  -Wl,-mllvm,-pass-remarks=wholeprogramdevirt -Wl,--lto-O1  -Wl,-save-temps -g -O2 -fsanitize-stats -fsanitize=cfi-vcall 2>&1 | c++filt

835 ↗

(On Diff #66998)

Thank you for the hints. Let's focus on the other issue (dups) first, and then I will update this comment.

[krasin]

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

615 ↗

(On Diff #66998)

After adding a virtual method with a single implementation, I've got the suggested behavior:

remark: devirt.cc:60:8: devirtualized call
remark: devirt.cc:20:0: i=0 devirtualized Base::singlePrint()
remark: devirt.cc:20:0: i=1 devirtualized Base::singlePrint()
remark: devirt.cc:20:0: i=2 devirtualized Base::singlePrint()
remark: devirt.cc:20:0: i=3 devirtualized Base::singlePrint()

We should definitely suppress such duplicates, as it's possible to do without creating a set.

[krasin]

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

835 ↗

(On Diff #66998)

Okay, I see the problem. Thanks a ton for spotting it!

[krasin]

llvm/trunk/lib/Transforms/IPO/WholeProgramDevirt.cpp

620 ↗

(On Diff #66998)

For the record: the follow up CL is https://reviews.llvm.org/D23297

lib/Transforms/IPO/WholeProgramDevirt.cpp

 #include "llvm/Transforms/IPO/WholeProgramDevirt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/Analysis/TypeMetadataUtils.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/IPO.h"
for (auto Call : CSByConstantArg.second) {
         Value *Val = B.CreateLoad(RetType, ValAddr);
         Call.replaceAndErase(Val);
       }
     }
   }
   return true;
 }
 
+static void emitTargetsRemarks(std::vector<VirtualCallTarget> &TargetsForSlot) {
+  for (const VirtualCallTarget &Target : TargetsForSlot) {
+    Function *F = Target.Fn;
+    DISubprogram *SP = F->getSubprogram();
+    DebugLoc DL = SP ? DebugLoc::get(SP->getScopeLine(), 0, SP) : DebugLoc();
+    emitOptimizationRemark(F->getContext(), DEBUG_TYPE, *F, DL,
+                           std::string("devirtualized ") + F->getName().str());
+  }
+}
+
 void DevirtModule::rebuildGlobal(VTableBits &B) {
   if (B.Before.Bytes.empty() && B.After.Bytes.empty())
     return;
 
   // Align each byte array to pointer width.
   unsigned PointerSize = M.getDataLayout().getPointerSize();
   B.Before.Bytes.resize(alignTo(B.Before.Bytes.size(), PointerSize));
   B.After.Bytes.resize(alignTo(B.After.Bytes.size(), PointerSize));
for (auto &S : CallSlots) {
     // Search each of the members of the type identifier for the virtual
     // function implementation at offset S.first.ByteOffset, and add to
     // TargetsForSlot.
     std::vector<VirtualCallTarget> TargetsForSlot;
     if (!tryFindVirtualCallTargets(TargetsForSlot, TypeIdMap[S.first.TypeID],
                                    S.first.ByteOffset))
       continue;
 
-    if (trySingleImplDevirt(TargetsForSlot, S.second))
+    if (trySingleImplDevirt(TargetsForSlot, S.second)) {
+      emitTargetsRemarks(TargetsForSlot);
       continue;
+    }
 
-    DidVirtualConstProp |= tryVirtualConstProp(TargetsForSlot, S.second);
+    if (tryVirtualConstProp(TargetsForSlot, S.second)) {
+      emitTargetsRemarks(TargetsForSlot);
+      DidVirtualConstProp = true;
+    }
   }
 
   // If we were able to eliminate all unsafe uses for a type checked load,
   // eliminate the type test by replacing it with true.
   if (TypeCheckedLoadFunc) {
     auto True = ConstantInt::getTrue(M.getContext());
     for (auto &&U : NumUnsafeUsesForTypeTest) {
       if (U.second == 0) {

test/Transforms/WholeProgramDevirt/devirt-single-impl.ll

-; RUN: opt -S -wholeprogramdevirt %s | FileCheck %s
+; RUN: opt -S -wholeprogramdevirt -pass-remarks=wholeprogramdevirt %s 2>&1 | FileCheck %s
 
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
+; CHECK: remark: <unknown>:0:0: devirtualized call
+; CHECK: remark: <unknown>:0:0: devirtualized vf
+; CHECK: remark: <unknown>:0:0: devirtualized vf
+
 @vt1 = constant [1 x i8*] [i8* bitcast (void (i8*)* @vf to i8*)], !type !0
 @vt2 = constant [1 x i8*] [i8* bitcast (void (i8*)* @vf to i8*)], !type !0
 
 define void @vf(i8* %this) {
   ret void
 }
 
 ; CHECK: define void @call

test/Transforms/WholeProgramDevirt/virtual-const-prop-check.ll

 ; RUN: opt -S -wholeprogramdevirt -pass-remarks=wholeprogramdevirt %s 2>&1 | FileCheck %s
 
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; CHECK: remark: <unknown>:0:0: devirtualized call
+; CHECK: remark: <unknown>:0:0: devirtualized vf1i32
+; CHECK: remark: <unknown>:0:0: devirtualized vf2i32
+; CHECK: remark: <unknown>:0:0: devirtualized vf3i32
+; CHECK: remark: <unknown>:0:0: devirtualized vf4i32
 ; CHECK: remark: <unknown>:0:0: devirtualized call
+; CHECK: remark: <unknown>:0:0: devirtualized vf1i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf0i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf1i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf0i1
 ; CHECK: remark: <unknown>:0:0: devirtualized call
+; CHECK: remark: <unknown>:0:0: devirtualized vf0i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf1i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf0i1
+; CHECK: remark: <unknown>:0:0: devirtualized vf1i1
 ; CHECK-NOT: devirtualized call
 
 ; CHECK: [[VT1DATA:@[^ ]*]] = private constant { [8 x i8], [3 x i8*], [0 x i8] } { [8 x i8] c"\00\00\00\01\01\00\00\00", [3 x i8*] [i8* bitcast (i1 (i8*)* @vf0i1 to i8*), i8* bitcast (i1 (i8*)* @vf1i1 to i8*), i8* bitcast (i32 (i8*)* @vf1i32 to i8*)], [0 x i8] zeroinitializer }, section "vt1sec", !type [[T8:![0-9]+]]
 @vt1 = constant [3 x i8*] [
 i8* bitcast (i1 (i8*)* @vf0i1 to i8*),
 i8* bitcast (i1 (i8*)* @vf1i1 to i8*),
 i8* bitcast (i32 (i8*)* @vf1i32 to i8*)
 ], section "vt1sec", !type !0