[WPD] Provide a way to prevent function(s) from being devirtualized

[tejohnson]

This won't work for the index-only WPD. Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

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

2

Why does skipping this one in particular cause both vf0i1 and vf1i1 to be skipped?

Also the comment here says that we won't devirtualize vf0i1 "and all other virtual call targets" except that the checking still looks for devirtualizations of vf*i32.

6

Similar question here about why skipping one blocks the devirt of the others

25

Should there be a SKIP-NOT: devirtualized above here too to make sure there are no devirt messages before this?

[evgeny777]

This won't work for the index-only WPD.

This can be addressed at the moment, because index based WPD supports only single-impl devirtualization. In this case we have function name in the index.

Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

I don't think this is enough. If we have call slot { foo, bar }, and want to prevent devirtualization of foo, then entire call slot can't be devirtualized. If bar is not used by any other call slot then it won't be devirtualized as well

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

2

We have 4 vtables:

vtbl1: { vf0i1, vf1i1, vf1i32 }
vtbl2: { vf1i1, vf0i1, vf2i32 }
vtbl3: { vf0i1, vf1i1, vf3i32 }
vtbl4: { vf1i1, vf0i1, vf4i32 }

and 3 call slots:

CS1: (ByteOffset == 16) : { vf1i32, vf2i32, vf3i32, vf4i32 }
CS2: (ByteOffset == 8) : { vf1i1, vf0i1 }
CS3: (ByteOffset == 0) : { vf0i1, vf1i1 }

If we want to prevent devirtualization of vf0i1 then both CS2 and CS3 can't be devirtualzied and final devirtualized set is { vf1i32, vf2i32, vf3i32, vf4i32 }
If we want to prevent devirtualization of vf1i32 then CS1 can't be devirtualized and final devirtualized set is { vf0i1 vf1i1 }

[tejohnson]

In D75617#1907097, @evgeny777 wrote:

This won't work for the index-only WPD.

This can be addressed at the moment, because index based WPD supports only single-impl devirtualization. In this case we have function name in the index.

Meaning we can just check the single impl name down in DevirtIndex::trySingleImplDevirt and block it there? Yeah, that will work.

Probably the best way to cover that case is to use the option in ModuleSummaryAnalysis.cpp to block functions from being added to the VTableFuncs in the summary in findFuncPointers.

I don't think this is enough. If we have call slot { foo, bar }, and want to prevent devirtualization of foo, then entire call slot can't be devirtualized. If bar is not used by any other call slot then it won't be devirtualized as well

Right, nevermind, we will actually do incorrect single impl devirts with what I suggested, because we will be missing some of the targets for the slot. I'm not sure I follow your second sentence here, but essentially if the targets for the slot are supposed to be {foo, bar} and if we didn't summarize foo, then we would incorrectly perform a single impl devirt to bar, when we shouldn't have devirtualized at all. Or are you saying something different?

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

2

Ok, nevermind, I was thinking about single impl devirt, wondering how we can devirtualize with multiple targets in the slot, but I see in this case it is because we are doing virtual constant prop.

[evgeny777]

if we didn't summarize foo, then we would incorrectly perform a single impl devirt to bar, when we shouldn't have devirtualized at all.

True

Or are you saying something different?

Actually I wanted to say that if we prevent foo from devirtualizing and have two call sites:

CS1: { foo, bar }
CS2: { bar, baz }

then CS2 will still be devirtulized, so specifying -wholeprogramdevirt-skip=foo is actually telling WPD that all call sites potentially calling foo should remain virtual.

[evgeny777]

• Added support for index based WPD
• Minor improvements in test case

[pcc]

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

[tejohnson]

In D75617#1908189, @pcc wrote:

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

I assumed debugging, but will let @evgeny777 comment on his use case.

[evgeny777]

There is already a way to do this, with the attribute [[clang::lto_visibility_public]]. Why do you need another one?

I think, it's not the same, because

• it requires full recompilation, not just relink.
• it operates on vtables not on individual functions

[tejohnson]

llvm/test/ThinLTO/X86/devirt.ll

128

This doesn't check for the absence of the _ZN1A1nEi devirt. Probably should be the following?
; SKIP-NOT: single-impl: devirtualized a call to _ZN1A1nEi

[evgeny777]

@pcc Just to clarify: I'm using this patch to identify problems with WPD (typically those are either invalid reinterpret_cast or open type hierarchy). I find it convenient to obtain a list of devirtualized functions using pass remarks, "undevirtualize" some of them relink and run. With attributes it seems to be much harder

[steven_wu]

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

[evgeny777]

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

It is for debugging. Not WPD itself but rather a devirtualized binary. See comment above

[pcc]

In D75617#1908240, @evgeny777 wrote:

@pcc Just to clarify: I'm using this patch to identify problems with WPD (typically those are either invalid reinterpret_cast or open type hierarchy). I find it convenient to obtain a list of devirtualized functions using pass remarks, "undevirtualize" some of them relink and run. With attributes it seems to be much harder

Okay. If it's just for debugging then the option needs to be made hidden.

[steven_wu]

In D75617#1908254, @evgeny777 wrote:

What is the goal for this option? Do you expect any users of this or this is for debugging WPD?

It is for debugging. Not WPD itself but rather a devirtualized binary. See comment above

Sorry I skipped ahead after reading the first few comments. That sounds fine to me. Thanks for explaining.

[evgeny777]

Addressed comments

[tejohnson]

LGTM

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp

 #include "llvm/Pass.h"
 #include "llvm/PassRegistry.h"
 #include "llvm/PassSupport.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/FunctionAttrs.h"
 #include "llvm/Transforms/Utils/Evaluator.h"
 #include <algorithm>
 #include <cstddef>
 #include <map>
 #include <set>

 
 /// Provide a way to force disable whole program for debugging or workarounds,
 /// when enabled via the linker.
 cl::opt<bool> DisableWholeProgramVisibility(
     "disable-whole-program-visibility", cl::init(false), cl::Hidden,
     cl::ZeroOrMore,
     cl::desc("Disable whole program visibility (overrides enabling options)"));
 
+/// Provide way to prevent certain function from being devirtualized
+cl::list<std::string>
+    SkipFunctionNames("wholeprogramdevirt-skip",
+                      cl::desc("Prevent function(s) from being devirtualized"),
+                      cl::Hidden, cl::ZeroOrMore, cl::CommaSeparated);
+
+namespace {
+struct PatternList {
+  std::vector<GlobPattern> Patterns;
+  template <class T> void init(const T &StringList) {
+    for (const auto &S : StringList)
+      if (Expected<GlobPattern> Pat = GlobPattern::create(S))
+        Patterns.push_back(std::move(*Pat));
+  }
+  bool match(StringRef S) {
+    for (const GlobPattern &P : Patterns)
+      if (P.match(S))
+        return true;
+    return false;
+  }
+};
+} // namespace
+
 // Find the minimum offset that we may store a value of size Size bits at. If
 // IsAfter is set, look for an offset before the object, otherwise look for an
 // offset after the object.
 uint64_t
 wholeprogramdevirt::findLowestOffset(ArrayRef<VirtualCallTarget> Targets,
                                      bool IsAfter, uint64_t Size) {
   // Find a minimum offset taking into account only vtable sizes.
   uint64_t MinByte = 0;
struct DevirtModule {
   // pointer. The key is the associated llvm.type.test intrinsic call generated
   // by this pass. An unsafe use is one that calls the loaded function pointer
   // directly. Every time we eliminate an unsafe use (for example, by
   // devirtualizing it or by applying virtual constant propagation), we
   // decrement the value stored in this map. If a value reaches zero, we can
   // eliminate the type check by RAUWing the associated llvm.type.test call with
   // true.
   std::map<CallInst *, unsigned> NumUnsafeUsesForTypeTest;
+  PatternList FunctionsToSkip;
 
   DevirtModule(Module &M, function_ref<AAResults &(Function &)> AARGetter,
                function_ref<OptimizationRemarkEmitter &(Function *)> OREGetter,
                function_ref<DominatorTree &(Function &)> LookupDomTree,
                ModuleSummaryIndex *ExportSummary,
                const ModuleSummaryIndex *ImportSummary)
       : M(M), AARGetter(AARGetter), LookupDomTree(LookupDomTree),
         ExportSummary(ExportSummary), ImportSummary(ImportSummary),
         Int8Ty(Type::getInt8Ty(M.getContext())),
         Int8PtrTy(Type::getInt8PtrTy(M.getContext())),
         Int32Ty(Type::getInt32Ty(M.getContext())),
         Int64Ty(Type::getInt64Ty(M.getContext())),
         IntPtrTy(M.getDataLayout().getIntPtrType(M.getContext(), 0)),
         RemarksEnabled(areRemarksEnabled()), OREGetter(OREGetter) {
     assert(!(ExportSummary && ImportSummary));
+    FunctionsToSkip.init(SkipFunctionNames);
   }
 
   bool areRemarksEnabled();
 
   void scanTypeTestUsers(Function *TypeTestFunc);
   void scanTypeCheckedLoadUsers(Function *TypeCheckedLoadFunc);
 
   void buildTypeIdentifierMap(
struct DevirtIndex {
   std::set<GlobalValue::GUID> &ExportedGUIDs;
   // A map in which to record the information necessary to locate the WPD
   // resolution for local targets in case they are exported by cross module
   // importing.
   std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap;
 
   MapVector<VTableSlotSummary, VTableSlotInfo> CallSlots;
 
+  PatternList FunctionsToSkip;
+
   DevirtIndex(
       ModuleSummaryIndex &ExportSummary,
       std::set<GlobalValue::GUID> &ExportedGUIDs,
       std::map<ValueInfo, std::vector<VTableSlotSummary>> &LocalWPDTargetsMap)
       : ExportSummary(ExportSummary), ExportedGUIDs(ExportedGUIDs),
-        LocalWPDTargetsMap(LocalWPDTargetsMap) {}
+        LocalWPDTargetsMap(LocalWPDTargetsMap) {
+    FunctionsToSkip.init(SkipFunctionNames);
+  }
 
   bool tryFindVirtualCallTargets(std::vector<ValueInfo> &TargetsForSlot,
                                  const TypeIdCompatibleVtableInfo TIdInfo,
                                  uint64_t ByteOffset);
 
   bool trySingleImplDevirt(MutableArrayRef<ValueInfo> TargetsForSlot,
                            VTableSlotSummary &SlotSummary,
                            VTableSlotInfo &SlotInfo,
Constant *Ptr = getPointerAtOffset(TM.Bits->GV->getInitializer(),
                                        TM.Offset + ByteOffset, M);
     if (!Ptr)
       return false;
 
     auto Fn = dyn_cast<Function>(Ptr->stripPointerCasts());
     if (!Fn)
       return false;
 
+    if (FunctionsToSkip.match(Fn->getName()))
+      return false;
+
     // We can disregard __cxa_pure_virtual as a possible call target, as
     // calls to pure virtuals are UB.
     if (Fn->getName() == "__cxa_pure_virtual")
       continue;
 
     TargetsForSlot.push_back({Fn, &TM});
   }
 
for (auto &&Target : TargetsForSlot)
     if (TheFn != Target)
       return false;
 
   // Don't devirtualize if we don't have target definition.
   auto Size = TheFn.getSummaryList().size();
   if (!Size)
     return false;
 
+  // Don't devirtualize function if we're told to skip it
+  // in -wholeprogramdevirt-skip.
+  if (FunctionsToSkip.match(TheFn.name()))
+    return false;
+
   // If the summary list contains multiple summaries where at least one is
   // a local, give up, as we won't know which (possibly promoted) name to use.
   for (auto &S : TheFn.getSummaryList())
     if (GlobalValue::isLocalLinkage(S->linkage()) && Size > 1)
       return false;
 
   // Collect functions devirtualized at least for one call site for stats.
   if (PrintSummaryDevirt)

llvm/test/ThinLTO/X86/devirt.ll

 ; RUN:   -r=%t2.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1C1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1D1mEi,p \
 ; RUN:   -r=%t2.o,_ZTV1B,px \
 ; RUN:   -r=%t2.o,_ZTV1C,px \
 ; RUN:   -r=%t2.o,_ZTV1D,px 2>&1 | FileCheck %s --check-prefix=REMARK
 ; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
+; Check that we're able to prevent specific function from being
+; devirtualized when running index based WPD.
+; RUN: llvm-lto2 run %t2.o -save-temps -pass-remarks=. \
+; RUN:   -whole-program-visibility \
+; RUN:   -wholeprogramdevirt-skip=_ZN1A1nEi \
+; RUN:   -o %t3 \
+; RUN:   -r=%t2.o,test,px \
+; RUN:   -r=%t2.o,_ZN1A1nEi,p \
+; RUN:   -r=%t2.o,_ZN1B1fEi,p \
+; RUN:   -r=%t2.o,_ZN1C1fEi,p \
+; RUN:   -r=%t2.o,_ZN1D1mEi,p \
+; RUN:   -r=%t2.o,_ZTV1B,px \
+; RUN:   -r=%t2.o,_ZTV1C,px \
+; RUN:   -r=%t2.o,_ZTV1D,px 2>&1 | FileCheck %s --check-prefix=SKIP
+
 ; New PM, Index based WPD
 ; RUN: llvm-lto2 run %t2.o -save-temps -use-new-pm -pass-remarks=. \
 ; RUN:   -whole-program-visibility \
 ; RUN:   -o %t3 \
 ; RUN:   -r=%t2.o,test,px \
 ; RUN:   -r=%t2.o,_ZN1A1nEi,p \
 ; RUN:   -r=%t2.o,_ZN1B1fEi,p \
 ; RUN:   -r=%t2.o,_ZN1C1fEi,p \

 ; RUN:   -r=%t.o,_ZTV1B,px \
 ; RUN:   -r=%t.o,_ZTV1C,px \
 ; RUN:   -r=%t.o,_ZTV1D,px 2>&1 | FileCheck %s --check-prefix=REMARK --dump-input=fail
 ; RUN: llvm-dis %t3.1.4.opt.bc -o - | FileCheck %s --check-prefix=CHECK-IR
 
 ; REMARK-DAG: single-impl: devirtualized a call to _ZN1A1nEi
 ; REMARK-DAG: single-impl: devirtualized a call to _ZN1D1mEi
 
+; SKIP-NOT: devirtualized a call to _ZN1A1nEi
+
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
 
 %struct.A = type { i32 (...)** }
 %struct.B = type { %struct.A }
 %struct.C = type { %struct.A }
 %struct.D = type { i32 (...)** }
 

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

 ; RUN: opt -S -wholeprogramdevirt -whole-program-visibility -pass-remarks=wholeprogramdevirt %s 2>&1 | FileCheck %s
+; Skipping vf0i1 is identical to setting public LTO visibility. We don't devirtualize vf0i1 and all other
+; virtual call targets.
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility -pass-remarks=wholeprogramdevirt -wholeprogramdevirt-skip=vf0i1 %s 2>&1 | FileCheck %s --check-prefix=SKIP
+; We have two set of call targets {vf0i1, vf1i1} and {vf1i32, vf2i32, vf3i32, vf4i32}.
+; The command below prevents both of them from devirtualization.
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility -pass-remarks=wholeprogramdevirt -wholeprogramdevirt-skip=vf0i1,vf1i32 %s 2>&1 | FileCheck %s --check-prefix=SKIP-ALL
+; Check wildcard
+; RUN: opt -S -wholeprogramdevirt -whole-program-visibility -pass-remarks=wholeprogramdevirt -wholeprogramdevirt-skip=vf?i1 %s 2>&1 | FileCheck %s --check-prefix=SKIP
 
 target datalayout = "e-p:64:64"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; CHECK: remark: <unknown>:0:0: virtual-const-prop: devirtualized a call to vf1i32
 ; CHECK: remark: <unknown>:0:0: virtual-const-prop-1-bit: devirtualized a call to vf1i1
 ; CHECK: remark: <unknown>:0:0: virtual-const-prop-1-bit: devirtualized a call to vf0i1
 ; CHECK: remark: <unknown>:0:0: devirtualized vf0i1
 ; CHECK: remark: <unknown>:0:0: devirtualized vf1i1
 ; 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-NOT: devirtualized
 
+; SKIP: remark: <unknown>:0:0: virtual-const-prop: devirtualized a call to vf1i32
+; SKIP: remark: <unknown>:0:0: devirtualized vf1i32
+; SKIP: remark: <unknown>:0:0: devirtualized vf2i32
+; SKIP: remark: <unknown>:0:0: devirtualized vf3i32
+; SKIP: remark: <unknown>:0:0: devirtualized vf4i32
+; SKIP-NOT: devirtualized
+
+; SKIP-ALL-NOT: devirtualized
+
 ; 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
 
 ; CHECK: [[VT2DATA:@[^ ]*]] = private constant { [8 x i8], [3 x i8*], [0 x i8] } { [8 x i8] c"\00\00\00\02\02\00\00\00", [3 x i8*] [i8* bitcast (i1 (i8*)* @vf1i1 to i8*), i8* bitcast (i1 (i8*)* @vf0i1 to i8*), i8* bitcast (i32 (i8*)* @vf2i32 to i8*)], [0 x i8] zeroinitializer }, !type [[T8]]