[LTO][WPD] Simplify mustBeUnreachableFunction and test after D115492

[MaskRay]

Summary

An well-formed IR function definition must have an entry basic block and
a well-formed IR basic block must have one terminator so the emptiness
check can be simplified.
Also simplify the test a bit.

[tejohnson]

Thanks for the simplifications. This lgtm but please wait for @luna to confirm that the simplified test case fails without the original fix.

[mingmingl]

thanks for the simplification!

The comment for IR (test case) is just a FYI, and for the cpp change, more of a double check since I wasn't sure myself if function declaration would make a difference.

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

242–243

An well-formed IR function definition must have an entry basic block and

a well-formed IR basic block must have one terminator so the emptiness
check can be simplified.

Thanks for pointing this out!

I think this is true for function definition but not true for function declarations. Yet I don't know if computeFunctionSummary [1] (caller of mustBeUnreachableFunction) guarantees it's a function definition (not a function declaration).

Is there a way to make this safe for function declarations (if necessary)?

[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L257

llvm/test/ThinLTO/X86/Inputs/devirt_hybrid_after_filtering_unreachable_lib.ll

37–39

thanks for the simplification!

The change looks good to me, and I can verify that nodevirtualization happens on simplified IRs without the fix (by using -vv on llvm-lit to print out the commands, and re-run commands using {opt, llvm-lto2} without the change.

Just fyi I added -O3 (which is absent before) on the command to generate the IR, and got simpler IR for both IR [1]; so going to carry on the simplification in a pending patch (https://reviews.llvm.org/D115648)

[1] https://gist.github.com/minglotus-6/aea3355e6e9a63d5a047a301dca7c7d7 for devirt_hybrid_after_filtering_unreachable.ll, and https://gist.github.com/minglotus-6/546e2a6627a8cc6823022b4ff30a7d43 for Inputs/devirt_hybrid_after_filtering_unreachable_lib.ll,

[mingmingl]

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

242–243

An well-formed IR function definition must have an entry basic block and

a well-formed IR basic block must have one terminator so the emptiness
check can be simplified.

Thanks for pointing this out!

I think this is true for function definition but not true for function declarations. Yet I don't know if computeFunctionSummary [1] (caller of mustBeUnreachableFunction) guarantees it's a function definition (not a function declaration).

Is there a way to make this safe for function declarations (if necessary)?

[1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L257

After another look, caller guarantees F is a function definition, so LGTM. Maybe add a comment for mustBeUnreachableFunction that caller guarantees F is a function definition (not a function declaration)?

• There is only one caller of computeFunctionSummary, and it skips function declarations (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L793)

[MaskRay]

add comment and assert

llvm/lib/Analysis/ModuleSummaryAnalysis.cpp

 
 static bool isNonVolatileStore(const Instruction *I) {
   if (const auto *SI = dyn_cast<StoreInst>(I))
     return !SI->isVolatile();
 
   return false;
 }
 
-// Returns true if `F` must be an unreachable function.
+// Returns true if the function definition must be unreachable.
 //
 // Note if this helper function returns true, `F` is guaranteed
 // to be unreachable; if it returns false, `F` might still
 // be unreachable but not covered by this helper function.
 static bool mustBeUnreachableFunction(const Function &F) {
-  if (!F.empty()) {
-    const BasicBlock &entryBlock = F.getEntryBlock();
-    // A function must be unreachable if its entry block
-    // ends with an 'unreachable'.
-    if (!entryBlock.empty()) {
-      const Instruction *inst = &(*entryBlock.rbegin());
-      if (inst->getOpcode() == Instruction::Unreachable) {
-        return true;
-      }
-    }
-  }
-  return false;
+  // A function must be unreachable if its entry block ends with an
+  // 'unreachable'.
+  assert(!F.isDeclaration());
+  return isa<UnreachableInst>(F.getEntryBlock().getTerminator());
 }
 
 static void computeFunctionSummary(
     ModuleSummaryIndex &Index, const Module &M, const Function &F,
     BlockFrequencyInfo *BFI, ProfileSummaryInfo *PSI, DominatorTree &DT,
     bool HasLocalsInUsedOrAsm, DenseSet<GlobalValue::GUID> &CantBePromoted,
     bool IsThinLTO,
     std::function<const StackSafetyInfo *(const Function &F)> GetSSICallback) {

llvm/test/ThinLTO/X86/Inputs/devirt_hybrid_after_filtering_unreachable_lib.ll

 @_ZTI4Base = linkonce_odr hidden constant { i8*, i8* } { i8* bitcast (i8** getelementptr inbounds (i8*, i8** @_ZTVN10__cxxabiv117__class_type_infoE, i64 2) to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @_ZTS4Base, i32 0, i32 0) }, comdat, align 8
 @_ZTI7Derived = hidden constant { i8*, i8*, i8* } { i8* bitcast (i8** getelementptr inbounds (i8*, i8** @_ZTVN10__cxxabiv120__si_class_type_infoE, i64 2) to i8*), i8* getelementptr inbounds ([9 x i8], [9 x i8]* @_ZTS7Derived, i32 0, i32 0), i8* bitcast ({ i8*, i8* }* @_ZTI4Base to i8*) }, align 8
 @.str.1 = private unnamed_addr constant [24 x i8] c"In Derived::~Derived()\0A\00", align 1
 @_ZTV4Base = linkonce_odr hidden unnamed_addr constant { [5 x i8*] } { [5 x i8*] [i8* null, i8* bitcast ({ i8*, i8* }* @_ZTI4Base to i8*), i8* bitcast (void (%class.Base*)* @_ZN4BaseD2Ev to i8*), i8* bitcast (void (%class.Base*)* @_ZN4BaseD0Ev to i8*), i8* bitcast (void ()* @__cxa_pure_virtual to i8*)] }, comdat, align 8, !type !0, !type !1, !vcall_visibility !4
 @.str.2 = private unnamed_addr constant [18 x i8] c"In Base::~Base()\0A\00", align 1
 
 define hidden void @_ZN7Derived1xEv(%class.Derived* nonnull align 8 dereferenceable(8) %this) unnamed_addr align 2 {
 entry:
-  %this.addr = alloca %class.Derived*, align 8
-  store %class.Derived* %this, %class.Derived** %this.addr, align 8
-  %this1 = load %class.Derived*, %class.Derived** %this.addr, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([12 x i8], [12 x i8]* @.str, i64 0, i64 0))
   ret void
 }
 
 declare dso_local i32 @printf(i8*, ...)
 
 define hidden void @_Z3fooP4Base(%class.Base* %b) {
 entry:

 }
 
 declare i1 @llvm.type.test(i8*, metadata)
 
 declare void @llvm.assume(i1 noundef)
 
 define linkonce_odr hidden void @_ZN7DerivedD2Ev(%class.Derived* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Derived*, align 8
-  store %class.Derived* %this, %class.Derived** %this.addr, align 8
-  %this1 = load %class.Derived*, %class.Derived** %this.addr, align 8
-  %0 = bitcast %class.Derived* %this1 to i32 (...)***
+  %0 = bitcast %class.Derived* %this to i32 (...)***
   store i32 (...)** bitcast (i8** getelementptr inbounds ({ [5 x i8*] }, { [5 x i8*] }* @_ZTV7Derived, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %0, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([24 x i8], [24 x i8]* @.str.1, i64 0, i64 0))
-  %1 = bitcast %class.Derived* %this1 to %class.Base*
+  %1 = bitcast %class.Derived* %this to %class.Base*
   call void @_ZN4BaseD2Ev(%class.Base* nonnull align 8 dereferenceable(8) %1)
   ret void
 }
 
 define linkonce_odr hidden void @_ZN7DerivedD0Ev(%class.Derived* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Derived*, align 8
-  store %class.Derived* %this, %class.Derived** %this.addr, align 8
-  %this1 = load %class.Derived*, %class.Derived** %this.addr, align 8
-  call void @_ZN7DerivedD2Ev(%class.Derived* nonnull align 8 dereferenceable(8) %this1)
-  %0 = bitcast %class.Derived* %this1 to i8*
+  call void @_ZN7DerivedD2Ev(%class.Derived* nonnull align 8 dereferenceable(8) %this)
+  %0 = bitcast %class.Derived* %this to i8*
   call void @_ZdlPv(i8* %0)
   ret void
 }
 
 define linkonce_odr hidden void @_ZN4BaseD2Ev(%class.Base* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Base*, align 8
-  store %class.Base* %this, %class.Base** %this.addr, align 8
-  %this1 = load %class.Base*, %class.Base** %this.addr, align 8
-  %0 = bitcast %class.Base* %this1 to i32 (...)***
+  %0 = bitcast %class.Base* %this to i32 (...)***
   store i32 (...)** bitcast (i8** getelementptr inbounds ({ [5 x i8*] }, { [5 x i8*] }* @_ZTV4Base, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %0, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([18 x i8], [18 x i8]* @.str.2, i64 0, i64 0))
   ret void
 }
 
 define linkonce_odr hidden void @_ZN4BaseD0Ev(%class.Base* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Base*, align 8
-  store %class.Base* %this, %class.Base** %this.addr, align 8
-  %this1 = load %class.Base*, %class.Base** %this.addr, align 8
   call void @llvm.trap()
   unreachable
 }
 
 declare dso_local void @__cxa_pure_virtual() unnamed_addr
 
 declare void @llvm.trap()
 
 declare dso_local void @_ZdlPv(i8*)
 
-!llvm.module.flags = !{!5, !6, !7, !8}
+!llvm.module.flags = !{!5}
 !0 = !{i64 16, !"_ZTS4Base"}
 !1 = !{i64 32, !"_ZTSM4BaseFvvE.virtual"}
 !2 = !{i64 16, !"_ZTS7Derived"}
 !3 = !{i64 32, !"_ZTSM7DerivedFvvE.virtual"}
 !4 = !{i64 1}
-!5 = !{i32 1, !"wchar_size", i32 4}
-!6 = !{i32 1, !"Virtual Function Elim", i32 0}
-!7 = !{i32 7, !"uwtable", i32 1}
-!8 = !{i32 7, !"frame-pointer", i32 2}
+!5 = !{i32 1, !"Virtual Function Elim", i32 0}

llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll

entry:
   call void @_Z3fooP4Base(%class.Base* %2)
   ret i32 0
 }
 
 declare dso_local nonnull i8* @_Znwm(i64)
 
 define linkonce_odr hidden void @_ZN7DerivedC2Ev(%class.Derived* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Derived*, align 8
-  store %class.Derived* %this, %class.Derived** %this.addr, align 8
-  %this1 = load %class.Derived*, %class.Derived** %this.addr, align 8
-  %0 = bitcast %class.Derived* %this1 to %class.Base*
+  %0 = bitcast %class.Derived* %this to %class.Base*
   call void @_ZN4BaseC2Ev(%class.Base* nonnull align 8 dereferenceable(8) %0)
-  %1 = bitcast %class.Derived* %this1 to i32 (...)***
+  %1 = bitcast %class.Derived* %this to i32 (...)***
   store i32 (...)** bitcast (i8** getelementptr inbounds ({ [5 x i8*] }, { [5 x i8*] }* @_ZTV7Derived, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %1, align 8
   ret void
 }
 
 declare dso_local void @_Z3fooP4Base(%class.Base*)
 
 define linkonce_odr hidden void @_ZN4BaseC2Ev(%class.Base* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Base*, align 8
-  store %class.Base* %this, %class.Base** %this.addr, align 8
-  %this1 = load %class.Base*, %class.Base** %this.addr, align 8
-  %0 = bitcast %class.Base* %this1 to i32 (...)***
+  %0 = bitcast %class.Base* %this to i32 (...)***
   store i32 (...)** bitcast (i8** getelementptr inbounds ({ [5 x i8*] }, { [5 x i8*] }* @_ZTV4Base, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %0, align 8
   ret void
 }
 
 define linkonce_odr hidden void @_ZN4BaseD2Ev(%class.Base* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Base*, align 8
-  store %class.Base* %this, %class.Base** %this.addr, align 8
-  %this1 = load %class.Base*, %class.Base** %this.addr, align 8
-  %0 = bitcast %class.Base* %this1 to i32 (...)***
+  %0 = bitcast %class.Base* %this to i32 (...)***
   store i32 (...)** bitcast (i8** getelementptr inbounds ({ [5 x i8*] }, { [5 x i8*] }* @_ZTV4Base, i32 0, inrange i32 0, i32 2) to i32 (...)**), i32 (...)*** %0, align 8
   %call = call i32 (i8*, ...) @printf(i8* getelementptr inbounds ([18 x i8], [18 x i8]* @.str, i64 0, i64 0))
   ret void
 }
 
 define linkonce_odr hidden void @_ZN4BaseD0Ev(%class.Base* nonnull align 8 dereferenceable(8) %this) unnamed_addr comdat align 2 {
 entry:
-  %this.addr = alloca %class.Base*, align 8
-  store %class.Base* %this, %class.Base** %this.addr, align 8
-  %this1 = load %class.Base*, %class.Base** %this.addr, align 8
   call void @llvm.trap()
   unreachable
 }
 
 declare dso_local void @__cxa_pure_virtual() unnamed_addr
 
 declare dso_local i32 @printf(i8*, ...)
 
 declare void @llvm.trap()
 
-!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.module.flags = !{!3}
 
 !0 = !{i64 16, !"_ZTS4Base"}
 !1 = !{i64 32, !"_ZTSM4BaseFvvE.virtual"}
 !2 = !{i64 1}
-!3 = !{i32 1, !"wchar_size", i32 4}
-!4 = !{i32 1, !"Virtual Function Elim", i32 0}
-!5 = !{i32 7, !"uwtable", i32 1}
-!6 = !{i32 7, !"frame-pointer", i32 2}
+!3 = !{i32 1, !"Virtual Function Elim", i32 0}