Migrate PGOMemOptSizeOpt to use new OptimizationRemarkEmitter Pass

[lenary]

Summary

Fixes PR33790.

This patch still needs a yaml-style test, which I shall write tomorrow

[lenary]

Two quick notes for feedback, I shall get started on working on a test.

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

30 ↗

(On Diff #108600)

This change was added by git clang-format, probably incorrectly.

384 ↗

(On Diff #108600)

Not sure this is the right pair of names for this remark, maybe I should use getMIName(MI) (which returns "memset"/"memcpy"/"memmove"/"unknown") for the second string as well?

[anemet]

LGTM, I suppose there is already a -pass-remark test for this. Of course it would be great to also have a YAML test.

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

30 ↗

(On Diff #108600)

It probably orders uppercase before lowercase. This is fine. I usually land changes like this in a commit separate from the functional change but it's fine either way in this case.

384 ↗

(On Diff #108600)

No, I think it's fine the way it is. I think that it's better to have static strings for the remark name/identifier.

One nice improvement would be to pass the intrinsic name as an attribute so that we'd get "Intrinsic: memset" in YAML with NV("Intrinsic", getMIName(MI)).

I don't think we have an override for the Argument ctor that would take a StringRef. You may need to add that. This is also fine as a follow-on.

[lenary]

• Address Review Feedback, Add Test

llvm/trunk/include/llvm/IR/DiagnosticInfo.h

struct Argument {
     StringRef Key;
     std::string Val;
     // If set, the debug location corresponding to the value.
     DiagnosticLocation Loc;
 
     explicit Argument(StringRef Str = "") : Key("String"), Val(Str) {}
     Argument(StringRef Key, const Value *V);
     Argument(StringRef Key, const Type *T);
+    Argument(StringRef Key, StringRef S);
     Argument(StringRef Key, int N);
     Argument(StringRef Key, int64_t N);
     Argument(StringRef Key, unsigned N);
     Argument(StringRef Key, uint64_t N);
     Argument(StringRef Key, bool B) : Key(Key), Val(B ? "true" : "false") {}
   };
 
   /// \p PassName is the name of the pass emitting this diagnostic. \p

llvm/trunk/lib/IR/DiagnosticInfo.cpp

 }
 
 DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, const Type *T)
     : Key(Key) {
   raw_string_ostream OS(Val);
   OS << *T;
 }
 
+DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, StringRef S)
+    : Key(Key), Val(S.str()) {}
+
 DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, int N)
     : Key(Key), Val(itostr(N)) {}
 
 DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, int64_t N)
     : Key(Key), Val(itostr(N)) {}
 
 DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, unsigned N)
     : Key(Key), Val(utostr(N)) {}

llvm/trunk/lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/GlobalsModRef.h"
+#include "llvm/Analysis/OptimizationDiagnosticInfo.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DerivedTypes.h"
-#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instruction.h"
 #include "llvm/IR/Instructions.h"
-#include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Type.h"
 #include "llvm/Pass.h"
 #include "llvm/PassRegistry.h"
 #include "llvm/PassSupport.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/Support/Casting.h"
public:
   }
 
   StringRef getPassName() const override { return "PGOMemOPSize"; }
 
 private:
   bool runOnFunction(Function &F) override;
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<BlockFrequencyInfoWrapperPass>();
+    AU.addRequired<OptimizationRemarkEmitterWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
   }
 };
 } // end anonymous namespace
 
 char PGOMemOPSizeOptLegacyPass::ID = 0;
 INITIALIZE_PASS_BEGIN(PGOMemOPSizeOptLegacyPass, "pgo-memop-opt",
                       "Optimize memory intrinsic using its size value profile",
                       false, false)
 INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass)
 INITIALIZE_PASS_END(PGOMemOPSizeOptLegacyPass, "pgo-memop-opt",
                     "Optimize memory intrinsic using its size value profile",
                     false, false)
 
 FunctionPass *llvm::createPGOMemOPSizeOptLegacyPass() {
   return new PGOMemOPSizeOptLegacyPass();
 }
 
 namespace {
 class MemOPSizeOpt : public InstVisitor<MemOPSizeOpt> {
 public:
-  MemOPSizeOpt(Function &Func, BlockFrequencyInfo &BFI)
-      : Func(Func), BFI(BFI), Changed(false) {
+  MemOPSizeOpt(Function &Func, BlockFrequencyInfo &BFI,
+               OptimizationRemarkEmitter &ORE)
+      : Func(Func), BFI(BFI), ORE(ORE), Changed(false) {
     ValueDataArray =
         llvm::make_unique<InstrProfValueData[]>(MemOPMaxVersion + 2);
     // Get the MemOPSize range information from option MemOPSizeRange,
     getMemOPSizeRangeFromOption(MemOPSizeRange, PreciseRangeStart,
                                 PreciseRangeLast);
   }
   bool isChanged() const { return Changed; }
   void perform() {
void visitMemIntrinsic(MemIntrinsic &MI) {
     if (dyn_cast<ConstantInt>(Length))
       return;
     WorkList.push_back(&MI);
   }
 
 private:
   Function &Func;
   BlockFrequencyInfo &BFI;
+  OptimizationRemarkEmitter &ORE;
   bool Changed;
   std::vector<MemIntrinsic *> WorkList;
   // Start of the previse range.
   int64_t PreciseRangeStart;
   // Last value of the previse range.
   int64_t PreciseRangeLast;
   // The space to read the profile annotation.
   std::unique_ptr<InstrProfValueData[]> ValueDataArray;
for (uint64_t SizeId : SizeIds) {
     DEBUG(dbgs() << *CaseBB << "\n");
   }
   setProfMetadata(Func.getParent(), SI, CaseCounts, MaxCount);
 
   DEBUG(dbgs() << *BB << "\n");
   DEBUG(dbgs() << *DefaultBB << "\n");
   DEBUG(dbgs() << *MergeBB << "\n");
 
-  emitOptimizationRemark(Func.getContext(), "memop-opt", Func,
-                         MI->getDebugLoc(),
-                         Twine("optimize ") + getMIName(MI) + " with count " +
-                             Twine(SumForOpt) + " out of " + Twine(TotalCount) +
-                             " for " + Twine(Version) + " versions");
+  {
+    using namespace ore;
+    ORE.emit(OptimizationRemark(DEBUG_TYPE, "memopt-opt", MI)
+             << "optimized " << NV("Intrinsic", StringRef(getMIName(MI)))
+             << " with count " << NV("Count", SumForOpt) << " out of "
+             << NV("Total", TotalCount) << " for " << NV("Versions", Version)
+             << " versions");
+  }
 
   return true;
 }
 } // namespace
 
-static bool PGOMemOPSizeOptImpl(Function &F, BlockFrequencyInfo &BFI) {
+static bool PGOMemOPSizeOptImpl(Function &F, BlockFrequencyInfo &BFI,
+                                OptimizationRemarkEmitter &ORE) {
   if (DisableMemOPOPT)
     return false;
 
   if (F.hasFnAttribute(Attribute::OptimizeForSize))
     return false;
-  MemOPSizeOpt MemOPSizeOpt(F, BFI);
+  MemOPSizeOpt MemOPSizeOpt(F, BFI, ORE);
   MemOPSizeOpt.perform();
   return MemOPSizeOpt.isChanged();
 }
 
 bool PGOMemOPSizeOptLegacyPass::runOnFunction(Function &F) {
   BlockFrequencyInfo &BFI =
       getAnalysis<BlockFrequencyInfoWrapperPass>().getBFI();
-  return PGOMemOPSizeOptImpl(F, BFI);
+  auto &ORE = getAnalysis<OptimizationRemarkEmitterWrapperPass>().getORE();
+  return PGOMemOPSizeOptImpl(F, BFI, ORE);
 }
 
 namespace llvm {
 char &PGOMemOPSizeOptID = PGOMemOPSizeOptLegacyPass::ID;
 
 PreservedAnalyses PGOMemOPSizeOpt::run(Function &F,
                                        FunctionAnalysisManager &FAM) {
   auto &BFI = FAM.getResult<BlockFrequencyAnalysis>(F);
-  bool Changed = PGOMemOPSizeOptImpl(F, BFI);
+  auto &ORE = FAM.getResult<OptimizationRemarkEmitterAnalysis>(F);
+  bool Changed = PGOMemOPSizeOptImpl(F, BFI, ORE);
   if (!Changed)
     return PreservedAnalyses::all();
   auto PA = PreservedAnalyses();
   PA.preserve<GlobalsAA>();
   return PA;
 }
 } // namespace llvm

llvm/trunk/test/Transforms/PGOProfile/memop_size_opt.ll

 ; RUN: opt < %s -passes=pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -S | FileCheck %s --check-prefix=MEMOP_OPT
-; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -S | FileCheck %s --check-prefix=MEMOP_OPT
+; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -pass-remarks-with-hotness -pass-remarks-output=%t.opt.yaml -S | FileCheck %s --check-prefix=MEMOP_OPT
+; RUN: FileCheck %s -input-file=%t.opt.yaml --check-prefix=YAML
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @foo(i8* %dst, i8* %src, i8* %dst2, i8* %src2, i32* %a, i32 %n) !prof !27 {
 entry:
   br label %for.cond
 

 !30 = !{!"VP", i32 1, i64 556, i64 1, i64 99, i64 2, i64 88, i64 3, i64 77, i64 9, i64 72, i64 4, i64 66, i64 5, i64 55, i64 6, i64 44, i64 7, i64 33, i64 8, i64 22}
 !31 = !{!"VP", i32 1, i64 556, i64 1, i64 99, i64 2, i64 88, i64 3, i64 77, i64 9, i64 72, i64 4, i64 66, i64 5, i64 55, i64 6, i64 44, i64 7, i64 33, i64 8, i64 22}
 
 declare void @llvm.lifetime.start(i64, i8* nocapture)
 
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i32, i1)
 
 declare void @llvm.lifetime.end(i64, i8* nocapture)
+
+; YAML:      --- !Passed
+; YAML-NEXT: Pass:            pgo-memop-opt
+; YAML-NEXT: Name:            memopt-opt
+; YAML-NEXT: Function:        foo
+; YAML-NEXT: Hotness:         0
+; YAML-NEXT: Args:
+; YAML-NEXT:   - String:          'optimized '
+; YAML-NEXT:   - Intrinsic:       memcpy
+; YAML-NEXT:   - String:          ' with count '
+; YAML-NEXT:   - Count:           '99'
+; YAML-NEXT:   - String:          ' out of '
+; YAML-NEXT:   - Total:           '556'
+; YAML-NEXT:   - String:          ' for '
+; YAML-NEXT:   - Versions:        '1'
+; YAML-NEXT:   - String:          ' versions'
+; YAML-NEXT: ...
+; YAML-NEXT: --- !Passed
+; YAML-NEXT: Pass:            pgo-memop-opt
+; YAML-NEXT: Name:            memopt-opt
+; YAML-NEXT: Function:        foo
+; YAML-NEXT: Hotness:         0
+; YAML-NEXT: Args:
+; YAML-NEXT:   - String:          'optimized '
+; YAML-NEXT:   - Intrinsic:       memcpy
+; YAML-NEXT:   - String:          ' with count '
+; YAML-NEXT:   - Count:           '99'
+; YAML-NEXT:   - String:          ' out of '
+; YAML-NEXT:   - Total:           '556'
+; YAML-NEXT:   - String:          ' for '
+; YAML-NEXT:   - Versions:        '1'
+; YAML-NEXT:   - String:          ' versions'
+; YAML-NEXT: ...