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

1250

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.

1288

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

1250

Agree. Replaced Pass3 with PassLast.

1288

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

[xur]

LGTM.

[hoy]

llvm/lib/CodeGen/TargetPassConfig.cpp

1289

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

1289

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.

1289

I'm fine with a warning too.

[shenhan]

llvm/lib/CodeGen/TargetPassConfig.cpp

1228

Deleted.

1289

Done by reporting warning only.

[wenlei]

lgtm, thanks.

llvm/lib/CodeGen/TargetPassConfig.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Discriminator.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/SaveAndRestore.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Target/CGPassBuilderOption.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/Scalar.h"
 #include "llvm/Transforms/Utils.h"
 #include <cassert>
 #include <optional>
 #include <string>
 
static cl::opt<GlobalISelAbortMode> EnableGlobalISelAbort(
     cl::desc("Enable abort calls when \"global\" instruction selection "
              "fails to lower/select an instruction"),
     cl::values(
         clEnumValN(GlobalISelAbortMode::Disable, "0", "Disable the abort"),
         clEnumValN(GlobalISelAbortMode::Enable, "1", "Enable the abort"),
         clEnumValN(GlobalISelAbortMode::DisableWithDiag, "2",
                    "Disable the abort but emit a diagnostic on failure")));
 
-// An option that disables inserting FS-AFDO discriminators before emit.
-// This is mainly for debugging and tuning purpose.
-static cl::opt<bool>
-    FSNoFinalDiscrim("fs-no-final-discrim", cl::init(false), cl::Hidden,
-                     cl::desc("Do not insert FS-AFDO discriminators before "
-                              "emit."));
 // Disable MIRProfileLoader before RegAlloc. This is for for debugging and
 // tuning purpose.
 static cl::opt<bool> DisableRAFSProfileLoader(
     "disable-ra-fsprofile-loader", cl::init(false), cl::Hidden,
     cl::desc("Disable MIRProfileLoader before RegAlloc"));
 // Disable MIRProfileLoader before BloackPlacement. This is for for debugging
 // and tuning purpose.
 static cl::opt<bool> DisableLayoutFSProfileLoader(
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) {
+    const std::string ProfileFile = getFSProfileFile(TM);
+    if (!ProfileFile.empty()) {
+      if (EnableFSDiscriminator) {
+        addPass(createMIRProfileLoaderPass(
+            ProfileFile, getFSRemappingFile(TM),
+            sampleprof::FSDiscriminatorPass::PassLast, nullptr));
+      } else {
+        // Sample profile is given, but FSDiscriminator is not
+        // enabled, this may result in performance regression.
+        WithColor::warning()
+            << "Using AutoFDO without FSDiscriminator for MFS may regress "
+               "performance.";
+      }
+    }
     addPass(createMachineFunctionSplitterPass());
   }
 
   if (!DisableCFIFixup && TM->Options.EnableCFIFixup)
     addPass(createCFIFixup());
 
   PM->add(createStackFrameLayoutAnalysisPass());
 
   // Add passes that directly emit MI after all other MI passes.
   addPreEmitPass2();
 
   AddingMachinePasses = false;
 }
 
 /// Add passes that optimize machine instructions in SSA form.
 void TargetPassConfig::addMachineSSAOptimization() {
   // Pre-ra tail duplication.
   addPass(&EarlyTailDuplicateID);
 
   // Optimize PHIs before DCE: removing dead PHI cycles may make more

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 2>&1 | FileCheck %s --check-prefix=NODISCRIMINATOR
+; NODISCRIMINATOR: warning: Using AutoFDO without FSDiscriminator for MFS may regress performance.
+
+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
+
+;; No profile is specified, no load passes.
+; 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 is specified, so we have first 2 load passes.
+; 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
+;; But mfs is not specified, so no "SampleFDO loader should be created"
+; NO-PROFILE-NOMFS: SampleFDO loader in MIR
+
+;; Profile is specified with mfs, so we have 3 load passes.
+; 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()