Part 2 of Fine tune MachineFunctionSplitPass (MFS) for FSAFDO.

[shenhan]

Summary

This is a follow up to D152399.

This CL adds a new discriminator pass. Also adds a new sample profile pass when MFS is enabled.

Also includes tests for both FSAFDO-MFS and FSADO pass config.

[xur]

llvm/lib/CodeGen/TargetPassConfig.cpp

1255

It seems that you use Pass3 to replace PassLast. Should we use PassLast here? This way we can still save Pass3 for others.
I don't think we will have other discriminator pass after this.

1279

The intention here is to use FSAFDO with improved discriminator for MFS.
I would suggest a warning (or even a hard error) here if we detect otherwise AFDO mode is used.
This would helpful to user mistakes (especially that we know MFS + AFDO can degrade the performance).

[snehasish]

If we commit this patch before D152399 then I think we can move the machine-function-splitter-fsafdo.ll test to D152399? It would be nice to keep the patch and it's own test in the same commit.

[shenhan]

llvm/lib/CodeGen/TargetPassConfig.cpp

1255

Agree. Replaced Pass3 with PassLast.

1279

Yes. Added a check for sample_profile+fs_discriminator must both present.

[xur]

LGTM.

[hoy]

llvm/lib/CodeGen/TargetPassConfig.cpp

1280

A fatal error seems too strong to me. Can we do a warning instead so that we can still experiment with non-FS AutoFDO profile?

[wenlei]

llvm/lib/CodeGen/TargetPassConfig.cpp

1228

Is this FSNoFinalDiscrim cl flag still used anywhere? Remove fs-no-final-discrim to cleanup?

[wenlei]

llvm/lib/CodeGen/TargetPassConfig.cpp

1280

Agree. There is no correctness issue, and sub-optimal performance doesn't justify a fatal.

[xur]

llvm/lib/CodeGen/TargetPassConfig.cpp

1228

Good catch. I don't think there is a use for the var. We should remove that option.

1280

I'm fine with a warning too.

[shenhan]

llvm/lib/CodeGen/TargetPassConfig.cpp

1228

Deleted.

1280

Done by reporting warning only.

[wenlei]

lgtm, thanks.

llvm/lib/CodeGen/TargetPassConfig.cpp

if (getOptLevel() != CodeGenOpt::None)
     addBlockPlacement();
 
   // Insert before XRay Instrumentation.
   addPass(&FEntryInserterID);
 
   addPass(&XRayInstrumentationID);
   addPass(&PatchableFunctionID);
 
-  if (EnableFSDiscriminator && !FSNoFinalDiscrim)
-    // Add FS discriminators here so that all the instruction duplicates
-    // in different BBs get their own discriminators. With this, we can "sum"
-    // the SampleFDO counters instead of using MAX. This will improve the
-    // SampleFDO profile quality.
-    addPass(createMIRAddFSDiscriminatorsPass(
-        sampleprof::FSDiscriminatorPass::PassLast));
-
   addPreEmitPass();
 
   if (TM->Options.EnableIPRA)
     // Collect register usage information and produce a register mask of
     // clobbered registers, to be used to optimize call sites.
     addPass(createRegUsageInfoCollector());
 
   // FIXME: Some backends are incompatible with running the verifier after
if (TM->Options.EnableMachineOutliner && getOptLevel() != CodeGenOpt::None &&
     bool RunOnAllFunctions =
         (EnableMachineOutliner == RunOutliner::AlwaysOutline);
     bool AddOutliner =
         RunOnAllFunctions || TM->Options.SupportsDefaultOutlining;
     if (AddOutliner)
       addPass(createMachineOutlinerPass(RunOnAllFunctions));
   }
 
+  if (EnableFSDiscriminator)
+    addPass(createMIRAddFSDiscriminatorsPass(
+        sampleprof::FSDiscriminatorPass::PassLast));
+
   // Machine function splitter uses the basic block sections feature. Both
   // cannot be enabled at the same time. Basic block sections takes precedence.
   // FIXME: In principle, BasicBlockSection::Labels and splitting can used
   // together. Update this check once we have addressed any issues.
   if (TM->getBBSectionsType() != llvm::BasicBlockSection::None) {
     if (TM->getBBSectionsType() == llvm::BasicBlockSection::List) {
       addPass(llvm::createBasicBlockSectionsProfileReaderPass(
           TM->getBBSectionsFuncListBuf()));
     }
     addPass(llvm::createBasicBlockSectionsPass());
   } else if (TM->Options.EnableMachineFunctionSplitter ||
              EnableMachineFunctionSplitter) {
-    addPass(createMachineFunctionSplitterPass());
+    bool HasAccurateSampleProfile = false;
+    const std::string ProfileFile = getFSProfileFile(TM);
+    if (!ProfileFile.empty()) {
+      if (EnableFSDiscriminator) {
+        HasAccurateSampleProfile = true;
+        addPass(createMIRProfileLoaderPass(
+            ProfileFile, getFSRemappingFile(TM),
+            sampleprof::FSDiscriminatorPass::PassLast, nullptr));
+      } else {
+        // Sample profile is given, but FSDiscriminator is not
+        // enabled, this may result in performance degradation.
+        report_fatal_error("Must use AutoFDO with FSDiscriminator for MFS.");
+      }
+    }
+    addPass(createMachineFunctionSplitterPass(HasAccurateSampleProfile));
   }
 
   if (!DisableCFIFixup && TM->Options.EnableCFIFixup)
     addPass(createCFIFixup());
 
   PM->add(createStackFrameLayoutAnalysisPass());
 
   // Add passes that directly emit MI after all other MI passes.

llvm/test/CodeGen/X86/load-sample-profile-2.ll

+;;; MFS with sample profile fails when no -enable-fs-discriminator=true.
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -fs-profile-file=%S/Inputs/fsloader-mfs.afdo -split-machine-functions -enable-fs-discriminator=false
+; XFAIL: *
+
+define void @foo4(i1 zeroext %0, i1 zeroext %1) nounwind {
+  br i1 %0, label %3, label %7
+
+3:
+  %4 = call i32 @bar()
+  br label %7
+
+5:
+  %6 = call i32 @baz()
+  br label %7
+
+7:
+  br i1 %1, label %8, label %10
+
+8:
+  %9 = call i32 @bam()
+  br label %12
+
+10:
+  %11 = call i32 @baz()
+  br label %12
+
+12:
+  %13 = tail call i32 @qux()
+  ret void
+}
+
+declare i32 @bar()
+declare i32 @baz()
+declare i32 @bam()
+declare i32 @qux()

llvm/test/CodeGen/X86/load-sample-profile.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -debug-pass=Structure -enable-fs-discriminator=true -improved-fs-discriminator=true 2>&1 | FileCheck %s --check-prefix=NOPROFILE
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -debug-pass=Structure -fs-profile-file=%S/Inputs/fsloader-mfs.afdo -enable-fs-discriminator=true -improved-fs-discriminator=true 2>&1 | FileCheck %s --check-prefix=PROFILE-NOMFS
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -debug-pass=Structure -fs-profile-file=%S/Inputs/fsloader-mfs.afdo -split-machine-functions -enable-fs-discriminator=true -improved-fs-discriminator=true 2>&1 | FileCheck %s --check-prefix=PROFILE-MFS
+
+; NOPROFILE: Add FS discriminators in MIR
+; NO-NOPROFILE: SampleFDO loader in MIR
+; NOPROFILE: Add FS discriminators in MIR
+; NO-NOPROFILE: SampleFDO loader in MIR
+; NOPROFILE: Add FS discriminators in MIR
+; NO-NOPROFILE: SampleFDO loader in MIR
+
+; PROFILE-NOMFS: Add FS discriminators in MIR
+; PROFILE-NOMFS: SampleFDO loader in MIR
+; PROFILE-NOMFS: Add FS discriminators in MIR
+; PROFILE-NOMFS: SampleFDO loader in MIR
+; PROFILE-NOMFS: Add FS discriminators in MIR
+;;;; mfs is not specified, so no "SampleFDO loader should be created"
+; NO-PROFILE-NOMFS: SampleFDO loader in MIR
+
+; PROFILE-MFS: Add FS discriminators in MIR
+; PROFILE-MFS: SampleFDO loader in MIR
+; PROFILE-MFS: Add FS discriminators in MIR
+; PROFILE-MFS: SampleFDO loader in MIR
+; PROFILE-MFS: Add FS discriminators in MIR
+; PROFILE-MFS: SampleFDO loader in MIR
+; PROFILE-MFS: Machine Function Splitter Transformation
+
+define void @foo4(i1 zeroext %0, i1 zeroext %1) nounwind {
+  br i1 %0, label %3, label %7
+
+3:
+  %4 = call i32 @bar()
+  br label %7
+
+5:
+  %6 = call i32 @baz()
+  br label %7
+
+7:
+  br i1 %1, label %8, label %10
+
+8:
+  %9 = call i32 @bam()
+  br label %12
+
+10:
+  %11 = call i32 @baz()
+  br label %12
+
+12:
+  %13 = tail call i32 @qux()
+  ret void
+}
+
+declare i32 @bar()
+declare i32 @baz()
+declare i32 @bam()
+declare i32 @qux()
+

llvm/test/CodeGen/X86/machine-function-splitter-fsafdo.ll

+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -enable-fs-discriminator -improved-fs-discriminator=true -split-machine-functions | FileCheck %s --check-prefix=FSAFDO-MFS
+
+; FSAFDO-MFS: .section  .text.split.foo4,"ax"
+; FSAFDO-MFS: foo4.cold:
+
+define void @foo4(i1 zeroext %0, i1 zeroext %1) nounwind !prof !20 {
+  br i1 %0, label %3, label %7, !prof !18
+
+3:
+  %4 = call i32 @bar()
+  br label %7
+
+5:
+  %6 = call i32 @baz()
+  br label %7
+
+7:
+  br i1 %1, label %8, label %10, !prof !19
+
+8:
+  %9 = call i32 @bam()
+  br label %12
+
+10:
+  %11 = call i32 @baz()
+  br label %12
+
+12:
+  %13 = tail call i32 @qux()
+  ret void
+}
+
+declare i32 @bar()
+declare i32 @baz()
+declare i32 @bam()
+declare i32 @qux()
+
+attributes #0 = { "implicit-section-name"="nosplit" }
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"ProfileSummary", !1}
+!1 = !{!2, !3, !4, !5, !6, !7, !8, !9}
+!2 = !{!"ProfileFormat", !"SampleProfile"}
+!3 = !{!"TotalCount", i64 10000}
+!4 = !{!"MaxCount", i64 10}
+!5 = !{!"MaxInternalCount", i64 1}
+!6 = !{!"MaxFunctionCount", i64 1000}
+!7 = !{!"NumCounts", i64 3}
+!8 = !{!"NumFunctions", i64 5}
+!9 = !{!"DetailedSummary", !10}
+!10 = !{!11, !12, !13}
+!11 = !{i32 10000, i64 100, i32 1}
+!12 = !{i32 999900, i64 100, i32 1}
+!13 = !{i32 999999, i64 1, i32 2}
+!14 = !{!"function_entry_count", i64 7000}
+!15 = !{!"function_section_prefix", !"hot"}
+!16 = !{!"function_section_prefix", !"unlikely"}
+!17 = !{!"branch_weights", i32 7000, i32 0}
+!18 = !{!"branch_weights", i32 0, i32 7000}
+!19 = !{!"branch_weights", i32 1000, i32 6000}
+!20 = !{!"function_entry_count", i64 10000}
+!21 = !{!"branch_weights", i32 6000, i32 4000}
+!22 = !{!"branch_weights", i32 80, i32 9920}
+!23 = !{!"function_entry_count", i64 7}