[GlobalOpt] Fix debug variance problem in hasOnlyColdCalls

[uabelho]

Summary

hasOnlyColdCalls skipped over calls to intrinsics, but it did so after
checking the linkage of the called function. This meant that the presence
of a call to a debug intrinsic could affect the outcome of the
optimization.

In my original reproducer (for an out of tree target) it was particularly
interesting, because the actual IR after GlobalOpt was not different with
debug instrinsics present, so -print-after-all printouts didn't show
anything there.

However, without debuginfo, GlobalOpt went further and ran
BlockFrequencyAnalysis and (more importanly) LoopAnalysis, and later on in
the pipeline, instcombine behaved in different ways when LoopInfo was
present.

So a call to a dbg.declare prevented running LoopAnalysis in
GlobalOpt, which later prevented InstCombine from doing an optimization.

The dbg-intrinsic-loopanalysis.ll testcase tries to expose this.

Then I also noted that adding a dbg.declare actually made the existing
testcase colccc_coldsites.ll generate different code, so I modified that
to now test it behaves the same way with and without the dbg.declare.

[uabelho]

llvm/lib/Transforms/IPO/GlobalOpt.cpp

1828

These checks were originally added in
https://reviews.llvm.org/D38413
but I suspect that checking if we're dealing with an intrinsic _after_ bailing out (with return false) on non-local linkage actually made the intrinsic check pointless.

So by changing the order I try to restore what I *think* was the purpose of the intrinsic check.

[nikic]

LGTM

However, without debuginfo, GlobalOpt went further and ran

BlockFrequencyAnalysis and (more importanly) LoopAnalysis, and later on in
the pipeline, instcombine behaved in different ways when LoopInfo was
present.

We should really stop doing that, and instead pass in an argument whether InstCombine in a given pipeline position should use LoopInfo or not. Especially with the NewPM "if available" dependencies are a really bad idea, because whether a given analysis is available or not can be quite unpredictable.

[fhahn]

Nice find! LGTM

[uabelho]

In D133193#3766538, @nikic wrote:

LGTM

However, without debuginfo, GlobalOpt went further and ran

BlockFrequencyAnalysis and (more importanly) LoopAnalysis, and later on in
the pipeline, instcombine behaved in different ways when LoopInfo was
present.

We should really stop doing that, and instead pass in an argument whether InstCombine in a given pipeline position should use LoopInfo or not. Especially with the NewPM "if available" dependencies are a really bad idea, because whether a given analysis is available or not can be quite unpredictable.

Yes this really had me confused for a while when I tried to track down where the debug variance occured.
Thanks for the super quick review!

llvm/lib/Transforms/IPO/GlobalOpt.cpp

for (BasicBlock &BB : F) {
     for (Instruction &I : BB) {
       if (CallInst *CI = dyn_cast<CallInst>(&I)) {
         // Skip over isline asm instructions since they aren't function calls.
         if (CI->isInlineAsm())
           continue;
         Function *CalledFn = CI->getCalledFunction();
         if (!CalledFn)
           return false;
-        if (!CalledFn->hasLocalLinkage())
-          return false;
         // Skip over intrinsics since they won't remain as function calls.
+        // Important to do this check before the linkage check below so we
+        // won't bail out on debug intrinsics, possibly making the generated
+        // code dependent on the presence of debug info.
         if (CalledFn->getIntrinsicID() != Intrinsic::not_intrinsic)
           continue;
+        if (!CalledFn->hasLocalLinkage())
+          return false;
         // Check if it's valid to use coldcc calling convention.
         if (!hasChangeableCC(CalledFn) || CalledFn->isVarArg() ||
             CalledFn->hasAddressTaken())
           return false;
         BlockFrequencyInfo &CallerBFI = GetBFI(F);
         if (!isColdCallSite(*CI, CallerBFI))
           return false;
       }

llvm/test/Transforms/GlobalOpt/PowerPC/coldcc_coldsites.ll

+; RUN: opt -strip-debug -S < %s | opt -passes=globalopt -mtriple=powerpc64le-unknown-linux-gnu -ppc-enable-coldcc -S | FileCheck %s -check-prefix=COLDCC
+; RUN: opt -strip-debug -S < %s | opt -passes=globalopt -S | FileCheck %s -check-prefix=CHECK
 ; RUN: opt -passes=globalopt -mtriple=powerpc64le-unknown-linux-gnu -ppc-enable-coldcc -S < %s | FileCheck %s -check-prefix=COLDCC
 ; RUN: opt -passes=globalopt -S < %s | FileCheck %s -check-prefix=CHECK
 
-define signext i32 @caller(i32 signext %a, i32 signext %b, i32 signext %lim, i32 signext %i) local_unnamed_addr #0 !prof !30 {
+define signext i32 @caller(i32 signext %a, i32 signext %b, i32 signext %lim, i32 signext %i, ptr %k) local_unnamed_addr #0 !prof !30 {
 entry:
 ; COLDCC: call coldcc signext i32 @callee
 ; CHECK: call fastcc signext i32 @callee
   %add = add nsw i32 %b, %a
   %sub = add nsw i32 %lim, -1
   %cmp = icmp eq i32 %sub, %i
   br i1 %cmp, label %if.then, label %if.end, !prof !31
 
 if.then:                                          ; preds = %entry
+  call void @llvm.dbg.declare(metadata ptr %k, metadata !101, metadata !DIExpression()), !dbg !122
   %call = tail call signext i32 @callee(i32 signext %a, i32 signext %b)
   br label %if.end
 
 if.end:                                           ; preds = %if.then, %entry
   %f.0 = phi i32 [ %call, %if.then ], [ %add, %entry ]
   ret i32 %f.0
 }
 
for.body:                                         ; preds = %for.body, %entry
   %i.011 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
   %ret.010 = phi i32 [ 0, %entry ], [ %add, %for.body ]
   %call = tail call signext i32 @caller(i32 signext 4, i32 signext 5, i32 signext 10000000, i32 signext %i.011)
   %add = add nsw i32 %call, %ret.010
   %inc = add nuw nsw i32 %i.011, 1
   %exitcond = icmp eq i32 %inc, 10000000
   br i1 %exitcond, label %for.cond.cleanup, label %for.body, !prof !34
 }
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)  #2
+
 attributes #0 = { noinline }
+attributes #2 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!117}
+!llvm.module.flags = !{!100}
 
 !0 = !{i32 1, !"ProfileSummary", !1}
 !1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
 !2 = !{!"ProfileFormat", !"InstrProf"}
 !3 = !{!"TotalCount", i64 20000003}
 !4 = !{!"MaxCount", i64 10000000}
 !5 = !{!"MaxInternalCount", i64 10000000}
 !6 = !{!"MaxFunctionCount", i64 10000000}

 !24 = !{i32 999900, i64 10000000, i32 2}
 !25 = !{i32 999990, i64 10000000, i32 2}
 !26 = !{i32 999999, i64 10000000, i32 2}
 !30 = !{!"function_entry_count", i64 10000000}
 !31 = !{!"branch_weights", i32 2, i32 10000000}
 !32 = !{i32 59}
 !33 = !{!"function_entry_count", i64 1}
 !34 = !{!"branch_weights", i32 2, i32 10000001}
+
+!100 = !{i32 2, !"Debug Info Version", i32 3}
+!101 = !DILocalVariable(name: "k", arg: 1, scope: !102, file: !103, line: 13, type: !119)
+!102 = distinct !DISubprogram(name: "h", scope: !103, file: !103, line: 13, type: !104, scopeLine: 13, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !117, retainedNodes: !121)
+!103 = !DIFile(filename: "foo2.c", directory: "/bar")
+!104 = !DISubroutineType(types: !105)
+!105 = !{!119}
+!117 = distinct !DICompileUnit(language: DW_LANG_C99, file: !103, producer: "clang version 16", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !118, globals: !120, splitDebugInlining: false, nameTableKind: None)
+!118 = !{!119}
+!119 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!120 = !{}
+!121 = !{!101}
+!122 = !DILocation(line: 13, column: 27, scope: !102)

llvm/test/Transforms/GlobalOpt/dbg-intrinsic-loopanalysis.ll

+; RUN: opt -passes="globalopt" < %s -o /dev/null -debug-pass-manager 2>&1 | FileCheck %s
+; RUN: opt -strip-debug -S < %s | opt -passes="globalopt" -o /dev/null -debug-pass-manager 2>&1 | FileCheck %s
+
+; Make sure that the call to dbg.declare does not prevent running BlockFrequency
+; and (especially) Loop Analysis.
+; Later passes (e.g. instcombine) may behave in different ways depending on if
+; LoopInfo is available or not. Therefore, letting GlobalOpt run or not run
+; LoopAnalysis depending on the presence of a dbg.declare may make the compiler
+; generate different code with and without debug info.
+
+; CHECK: Running pass: GlobalOptPass on [module]
+; CHECK: Running analysis: BlockFrequencyAnalysis on h
+; CHECK: Running analysis: LoopAnalysis on h
+
+define i16 @h(ptr %k) {
+entry:
+  call void @llvm.dbg.declare(metadata ptr %k, metadata !1, metadata !DIExpression()), !dbg !22
+  %call = call i16 @gaz()
+  ret i16 %call
+}
+
+; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.declare(metadata, metadata, metadata)  #0
+
+define internal i16 @gaz() {
+entry:
+  ret i16 0
+}
+
+attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!17}
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = !DILocalVariable(name: "k", arg: 1, scope: !2, file: !3, line: 13, type: !19)
+!2 = distinct !DISubprogram(name: "h", scope: !3, file: !3, line: 13, type: !4, scopeLine: 13, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !17, retainedNodes: !21)
+!3 = !DIFile(filename: "foo2.c", directory: "/bar")
+!4 = !DISubroutineType(types: !5)
+!5 = !{!19}
+!17 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 16", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !18, globals: !20, splitDebugInlining: false, nameTableKind: None)
+!18 = !{!19}
+!19 = !DIBasicType(name: "int", size: 16, encoding: DW_ATE_signed)
+!20 = !{}
+!21 = !{!1}
+!22 = !DILocation(line: 13, column: 27, scope: !2)