[OptDiag] Split code region out of DiagnosticInfoOptimizationBase

[anemet]

Summary

Code region is the only part of this class that is IR-specific. Code
region is moved down in the inheritance tree to a new derived class,
called DiagnosticInfoIROptimization.

All the existing remarks are derived from this new class now.

This allows the new MIR pass-remark classes to be derived from
DiagnosticInfoOptimizationBase.

Also because we keep the name DiagnosticInfoOptimizationBase, the clang
parts don't need any adjustment.

[MatzeB]

LGTM. Straightforward so not much to say except nitpicking about comments and names.

include/llvm/Analysis/OptimizationDiagnosticInfo.h

71–73

Probably no need to justify the choice of class here.

Instead I wish the documentation comment would tell me what the function actually does instead of just telling me it is the new way to emit things.

include/llvm/IR/DiagnosticInfo.h

381

Why not call this DiagnosticInfoOptimization to make it a bit shorter and call the new thing DiagnosticInfoIROptimization/DiagnosticInfoMIROptimization (the fact that it is a Base class or Common is not that important that it needs to go into the name IMO).

478

... used by (or in) IR passes

lib/Analysis/OptimizationDiagnosticInfo.cpp

152

Could use a reference instead of a pointer.

157

dito.

[anemet]

Thanks, Matthias!

include/llvm/IR/DiagnosticInfo.h

381

Good suggestion. Let me rename DiagnosticInfoOptimizationBase to DiagnosticInfoIROptimization in a prequel to this and rebase.

[anemet]

lib/Analysis/OptimizationDiagnosticInfo.cpp

157

Actually, this one needs to be a pointer because << requires an lvalue so we can't pass a temporary (&P).

[anemet]

The main thing is the renaming here proposed by Matthias.

I've actually kept the original name DiagnosticInfoOptimizationBase for the
IR-agnostic class (i.e. no code-region). This is nice because 99% of the
clients work because they weren't using code region. This includes clang so
there is no cross-repo renaming.

The other names are as suggested by Matthias.

Will also update the title and description of the review because it's
confusing now.

[MatzeB]

Still good to go.

lib/Analysis/OptimizationDiagnosticInfo.cpp

157

I am pretty sure this is equivalent and taking the address of a reference variable does produce a pointer that can still be used when the reference variable is out of scope (as long as the referenced object is still around which is the case here):

auto &P = const_cast<DiagnosticInfoOptimizationBase&>(OptDiagBase);
*out << &P;

[anemet]

Thanks.

lib/Analysis/OptimizationDiagnosticInfo.cpp

157

I am pretty it's not ;) since that is what I tried. &P is an rvalue for a pointer type so we can't take a reference of it.

Here is the actual error message:

../lib/Analysis/OptimizationDiagnosticInfo.cpp:161:10: error: invalid operands to binary expression ('yaml::Output' and 'llvm::DiagnosticInfoOptimizationBase *')
    *Out << &P;
    ~~~~ ^  ~~
../include/llvm/Support/YAMLTraits.h:1473:1: note: candidate function [with T = llvm::DiagnosticInfoOptimizationBase *] not viable: no known conversion from 'llvm::DiagnosticInfoOptimizationBase *' to 'llvm::DiagnosticInfoOptimizationBase *&' for 2nd argument

Arguably, operator<< should take a reference not a pointer. I forgot why I used a pointer here.

[MatzeB]

lib/Analysis/OptimizationDiagnosticInfo.cpp

157

operator<< needs an lvalue?!? Oh well, that is for another patch to clenaup/ complain about then.

include/llvm/Analysis/OptimizationDiagnosticInfo.h

OptimizationRemarkEmitter &operator=(OptimizationRemarkEmitter &&RHS) {
     return *this;
   }
 
   /// Handle invalidation events in the new pass manager.
   bool invalidate(Function &F, const PreservedAnalyses &PA,
                   FunctionAnalysisManager::Invalidator &Inv);
 
   /// The new interface to emit remarks.
-  void emit(DiagnosticInfoOptimizationBase &OptDiag);
+  ///
+  /// This takes a ...CommonBase rather than a ...Base because that is what
+  /// operator<< returns.
+  void emit(DiagnosticInfoOptimizationCommonBase &OptDiag);
 
   /// Emit an optimization-applied message.
   ///
   /// \p PassName is the name of the pass emitting the message. If -Rpass= is
   /// given and \p PassName matches the regular expression in -Rpass, then the
   /// remark will be emitted. \p Fn is the function triggering the remark, \p
   /// DLoc is the debug location where the diagnostic is generated. \p V is the
   /// IR Value that identifies the code region. \p Msg is the message string to

include/llvm/IR/DiagnosticInfo.h

 private:
   /// Function where this diagnostic is triggered.
   const Function &Fn;
 
   /// Debug location where this diagnostic is triggered.
   DebugLoc DLoc;
 };
 
-/// Common features for diagnostics dealing with optimization remarks.
-class DiagnosticInfoOptimizationBase : public DiagnosticInfoWithDebugLocBase {
+/// \brief Common features for diagnostics dealing with optimization remarks
+/// that are used by both IR and MIR passes.
+class DiagnosticInfoOptimizationCommonBase
+    : public DiagnosticInfoWithDebugLocBase {
 public:
   /// \brief Used to set IsVerbose via the stream interface.
   struct setIsVerbose {};
 
   /// \brief When an instance of this is inserted into the stream, the arguments
   /// following will not appear in the remark printed in the compiler output
   /// (-Rpass) but only in the optimization record file
   /// (-fsave-optimization-record).
struct Argument {
     Argument(StringRef Key, unsigned N);
     Argument(StringRef Key, bool B) : Key(Key), Val(B ? "true" : "false") {}
   };
 
   /// \p PassName is the name of the pass emitting this diagnostic. \p
   /// RemarkName is a textual identifier for the remark.  \p Fn is the function
   /// where the diagnostic is being emitted. \p DLoc is the location information
   /// to use in the diagnostic. If line table information is available, the
-  /// diagnostic will include the source code location. \p CodeRegion is IR
-  /// value (currently basic block) that the optimization operates on.  This is
-  /// currently used to provide run-time hotness information with PGO.
-  DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
-                                 enum DiagnosticSeverity Severity,
-                                 const char *PassName, StringRef RemarkName,
-                                 const Function &Fn, const DebugLoc &DLoc,
-                                 Value *CodeRegion = nullptr)
-      : DiagnosticInfoWithDebugLocBase(Kind, Severity, Fn, DLoc),
-        PassName(PassName), RemarkName(RemarkName), CodeRegion(CodeRegion) {}
-
-  /// \brief This is ctor variant allows a pass to build an optimization remark
-  /// from an existing remark.
-  ///
-  /// This is useful when a transformation pass (e.g LV) wants to emit a remark
-  /// (\p Orig) generated by one of its analyses (e.g. LAA) as its own analysis
-  /// remark.  The string \p Prepend will be emitted before the original
-  /// message.
-  DiagnosticInfoOptimizationBase(const char *PassName, StringRef Prepend,
-                                 const DiagnosticInfoOptimizationBase &Orig)
-      : DiagnosticInfoWithDebugLocBase((DiagnosticKind)Orig.getKind(),
-                                       Orig.getSeverity(), Orig.getFunction(),
-                                       Orig.getDebugLoc()),
-        PassName(PassName), RemarkName(Orig.RemarkName),
-        CodeRegion(Orig.getCodeRegion()) {
-    *this << Prepend;
-    std::copy(Orig.Args.begin(), Orig.Args.end(), std::back_inserter(Args));
-  }
-
-  /// Legacy interface.
-  /// \p PassName is the name of the pass emitting this diagnostic.
-  /// \p Fn is the function where the diagnostic is being emitted. \p DLoc is
-  /// the location information to use in the diagnostic. If line table
-  /// information is available, the diagnostic will include the source code
-  /// location. \p Msg is the message to show. Note that this class does not
-  /// copy this message, so this reference must be valid for the whole life time
-  /// of the diagnostic.
-  DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
+  /// diagnostic will include the source code location.
+  DiagnosticInfoOptimizationCommonBase(enum DiagnosticKind Kind,
                                        enum DiagnosticSeverity Severity,
-                                 const char *PassName, const Function &Fn,
-                                 const DebugLoc &DLoc, const Twine &Msg,
-                                 Optional<uint64_t> Hotness = None)
+                                       const char *PassName,
+                                       StringRef RemarkName, const Function &Fn,
+                                       const DebugLoc &DLoc)
       : DiagnosticInfoWithDebugLocBase(Kind, Severity, Fn, DLoc),
-        PassName(PassName), Hotness(Hotness) {
-    Args.push_back(Argument(Msg.str()));
-  }
+        PassName(PassName), RemarkName(RemarkName) {}
 
-  DiagnosticInfoOptimizationBase &operator<<(StringRef S);
-  DiagnosticInfoOptimizationBase &operator<<(Argument A);
-  DiagnosticInfoOptimizationBase &operator<<(setIsVerbose V);
-  DiagnosticInfoOptimizationBase &operator<<(setExtraArgs EA);
+  DiagnosticInfoOptimizationCommonBase &operator<<(StringRef S);
+  DiagnosticInfoOptimizationCommonBase &operator<<(Argument A);
+  DiagnosticInfoOptimizationCommonBase &operator<<(setIsVerbose V);
+  DiagnosticInfoOptimizationCommonBase &operator<<(setExtraArgs EA);
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
 
   /// Return true if this optimization remark is enabled by one of
   /// of the LLVM command line flags (-pass-remarks, -pass-remarks-missed,
   /// or -pass-remarks-analysis). Note that this only handles the LLVM
   /// flags. We cannot access Clang flags from here (they are handled
   /// in BackendConsumer::OptimizationRemarkHandler).
   virtual bool isEnabled() const = 0;
 
   StringRef getPassName() const { return PassName; }
   std::string getMsg() const;
   Optional<uint64_t> getHotness() const { return Hotness; }
   void setHotness(Optional<uint64_t> H) { Hotness = H; }
 
-  Value *getCodeRegion() const { return CodeRegion; }
-
   bool isVerbose() const { return IsVerbose; }
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() >= DK_FirstRemark &&
            DI->getKind() <= DK_LastRemark;
   }
 
-private:
+protected:
   /// Name of the pass that triggers this report. If this matches the
   /// regular expression given in -Rpass=regexp, then the remark will
   /// be emitted.
   const char *PassName;
 
   /// Textual identifier for the remark.  Can be used by external tools reading
   /// the YAML output file for optimization remarks to identify the remark.
   StringRef RemarkName;
 
   /// If profile information is available, this is the number of times the
   /// corresponding code was executed in a profile instrumentation run.
   Optional<uint64_t> Hotness;
 
-  /// The IR value (currently basic block) that the optimization operates on.
-  /// This is currently used to provide run-time hotness information with PGO.
-  Value *CodeRegion;
-
   /// Arguments collected via the streaming interface.
   SmallVector<Argument, 4> Args;
 
   /// The remark is expected to be noisy.
   bool IsVerbose = false;
 
   /// \brief If positive, the index of the first argument that only appear in
   /// the optimization records and not in the remark printed in the compiler
   /// output.
   int FirstExtraArgIndex = -1;
 
-  friend struct yaml::MappingTraits<DiagnosticInfoOptimizationBase *>;
+  friend struct yaml::MappingTraits<DiagnosticInfoOptimizationCommonBase *>;
+};
+
+/// \brief Common features for diagnostics dealing with optimization remarks
+/// that are used IR passes.
+class DiagnosticInfoOptimizationBase
+    : public DiagnosticInfoOptimizationCommonBase {
+public:
+  /// \p PassName is the name of the pass emitting this diagnostic. \p
+  /// RemarkName is a textual identifier for the remark.  \p Fn is the function
+  /// where the diagnostic is being emitted. \p DLoc is the location information
+  /// to use in the diagnostic. If line table information is available, the
+  /// diagnostic will include the source code location. \p CodeRegion is IR
+  /// value (currently basic block) that the optimization operates on.  This is
+  /// currently used to provide run-time hotness information with PGO.
+  DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
+                                 enum DiagnosticSeverity Severity,
+                                 const char *PassName, StringRef RemarkName,
+                                 const Function &Fn, const DebugLoc &DLoc,
+                                 Value *CodeRegion = nullptr)
+      : DiagnosticInfoOptimizationCommonBase(Kind, Severity, PassName,
+                                             RemarkName, Fn, DLoc),
+        CodeRegion(CodeRegion) {}
+
+  /// \brief This is ctor variant allows a pass to build an optimization remark
+  /// from an existing remark.
+  ///
+  /// This is useful when a transformation pass (e.g LV) wants to emit a remark
+  /// (\p Orig) generated by one of its analyses (e.g. LAA) as its own analysis
+  /// remark.  The string \p Prepend will be emitted before the original
+  /// message.
+  DiagnosticInfoOptimizationBase(const char *PassName, StringRef Prepend,
+                                 const DiagnosticInfoOptimizationBase &Orig)
+      : DiagnosticInfoOptimizationCommonBase(
+            (DiagnosticKind)Orig.getKind(), Orig.getSeverity(), PassName,
+            Orig.RemarkName, Orig.getFunction(), Orig.getDebugLoc()),
+        CodeRegion(Orig.getCodeRegion()) {
+    *this << Prepend;
+    std::copy(Orig.Args.begin(), Orig.Args.end(), std::back_inserter(Args));
+  }
+
+  /// Legacy interface.
+  /// \p PassName is the name of the pass emitting this diagnostic.
+  /// \p Fn is the function where the diagnostic is being emitted. \p DLoc is
+  /// the location information to use in the diagnostic. If line table
+  /// information is available, the diagnostic will include the source code
+  /// location. \p Msg is the message to show. Note that this class does not
+  /// copy this message, so this reference must be valid for the whole life time
+  /// of the diagnostic.
+  DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
+                                 enum DiagnosticSeverity Severity,
+                                 const char *PassName, const Function &Fn,
+                                 const DebugLoc &DLoc, const Twine &Msg,
+                                 Optional<uint64_t> Hotness = None)
+      : DiagnosticInfoOptimizationCommonBase(Kind, Severity, PassName, "", Fn,
+                                             DLoc) {
+    setHotness(Hotness);
+    *this << Msg.str();
+  }
+
+  Value *getCodeRegion() const { return CodeRegion; }
+
+private:
+  /// The IR value (currently basic block) that the optimization operates on.
+  /// This is currently used to provide run-time hotness information with PGO.
+  Value *CodeRegion;
 };
 
 /// Diagnostic information for applied optimization remarks.
 class OptimizationRemark : public DiagnosticInfoOptimizationBase {
 public:
   /// \p PassName is the name of the pass emitting this diagnostic. If
   /// this name matches the regular expression given in -Rpass=, then the
   /// diagnostic will be emitted. \p Fn is the function where the diagnostic

lib/Analysis/OptimizationDiagnosticInfo.cpp

if (!BFI)
     return None;
 
   return BFI->getBlockProfileCount(cast<BasicBlock>(V));
 }
 
 namespace llvm {
 namespace yaml {
 
-template <> struct MappingTraits<DiagnosticInfoOptimizationBase *> {
-  static void mapping(IO &io, DiagnosticInfoOptimizationBase *&OptDiag) {
+template <> struct MappingTraits<DiagnosticInfoOptimizationCommonBase *> {
+  static void mapping(IO &io, DiagnosticInfoOptimizationCommonBase *&OptDiag) {
     assert(io.outputting() && "input not yet implemented");
 
     if (io.mapTag("!Passed", OptDiag->getKind() == DK_OptimizationRemark))
       ;
     else if (io.mapTag("!Missed",
                        OptDiag->getKind() == DK_OptimizationRemarkMissed))
       ;
     else if (io.mapTag("!Analysis",
static void mapping(IO &io, DebugLoc &DL) {
     io.mapRequired("Line", Line);
     io.mapRequired("Column", Col);
   }
 
   static const bool flow = true;
 };
 
 // Implement this as a mapping for now to get proper quotation for the value.
-template <> struct MappingTraits<DiagnosticInfoOptimizationBase::Argument> {
-  static void mapping(IO &io, DiagnosticInfoOptimizationBase::Argument &A) {
+template <>
+struct MappingTraits<DiagnosticInfoOptimizationCommonBase::Argument> {
+  static void mapping(IO &io,
+                      DiagnosticInfoOptimizationCommonBase::Argument &A) {
     assert(io.outputting() && "input not yet implemented");
     io.mapRequired(A.Key.data(), A.Val);
     if (A.DLoc)
       io.mapOptional("DebugLoc", A.DLoc);
   }
 };
 
 } // end namespace yaml
 } // end namespace llvm
 
-LLVM_YAML_IS_SEQUENCE_VECTOR(DiagnosticInfoOptimizationBase::Argument)
+LLVM_YAML_IS_SEQUENCE_VECTOR(DiagnosticInfoOptimizationCommonBase::Argument)
 
 void OptimizationRemarkEmitter::computeHotness(
     DiagnosticInfoOptimizationBase &OptDiag) {
   Value *V = OptDiag.getCodeRegion();
   if (V)
     OptDiag.setHotness(computeHotness(V));
 }
 
-void OptimizationRemarkEmitter::emit(DiagnosticInfoOptimizationBase &OptDiag) {
-  computeHotness(OptDiag);
+void OptimizationRemarkEmitter::emit(
+    DiagnosticInfoOptimizationCommonBase &OptDiagCommon) {
+  auto *OptDiag = cast<DiagnosticInfoOptimizationBase>(&OptDiagCommon);
+  computeHotness(*OptDiag);
 
   yaml::Output *Out = F->getContext().getDiagnosticsOutputFile();
   if (Out) {
-    auto *P = &const_cast<DiagnosticInfoOptimizationBase &>(OptDiag);
+    auto *P =
+        const_cast<DiagnosticInfoOptimizationCommonBase *>(&OptDiagCommon);
     *Out << P;
   }
   // FIXME: now that IsVerbose is part of DI, filtering for this will be moved
   // from here to clang.
-  if (!OptDiag.isVerbose() || shouldEmitVerbose())
-    F->getContext().diagnose(OptDiag);
+  if (!OptDiag->isVerbose() || shouldEmitVerbose())
+    F->getContext().diagnose(*OptDiag);
 }
 
 void OptimizationRemarkEmitter::emitOptimizationRemark(const char *PassName,
                                                        const DebugLoc &DLoc,
                                                        const Value *V,
                                                        const Twine &Msg) {
   LLVMContext &Ctx = F->getContext();
   Ctx.diagnose(OptimizationRemark(PassName, *F, DLoc, Msg, computeHotness(V)));

lib/IR/DiagnosticInfo.cpp

const std::string DiagnosticInfoWithDebugLocBase::getLocationStr() const {
   StringRef Filename("<unknown>");
   unsigned Line = 0;
   unsigned Column = 0;
   if (isLocationAvailable())
     getLocation(&Filename, &Line, &Column);
   return (Filename + ":" + Twine(Line) + ":" + Twine(Column)).str();
 }
 
-DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, Value *V)
+DiagnosticInfoOptimizationCommonBase::Argument::Argument(StringRef Key,
+                                                         Value *V)
     : Key(Key) {
   if (auto *F = dyn_cast<Function>(V)) {
     if (DISubprogram *SP = F->getSubprogram())
       DLoc = DebugLoc::get(SP->getScopeLine(), 0, SP);
   }
   else if (auto *I = dyn_cast<Instruction>(V))
     DLoc = I->getDebugLoc();
 
   // Only include names that correspond to user variables.  FIXME: we should use
   // debug info if available to get the name of the user variable.
   if (isa<llvm::Argument>(V) || isa<GlobalValue>(V))
     Val = GlobalValue::getRealLinkageName(V->getName());
   else if (isa<Constant>(V)) {
     raw_string_ostream OS(Val);
     V->printAsOperand(OS, /*PrintType=*/false);
   } else if (auto *I = dyn_cast<Instruction>(V))
     Val = I->getOpcodeName();
 }
 
-DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, Type *T)
+DiagnosticInfoOptimizationCommonBase::Argument::Argument(StringRef Key, Type *T)
     : Key(Key) {
   raw_string_ostream OS(Val);
   OS << *T;
 }
 
-DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, int N)
+DiagnosticInfoOptimizationCommonBase::Argument::Argument(StringRef Key, int N)
     : Key(Key), Val(itostr(N)) {}
 
-DiagnosticInfoOptimizationBase::Argument::Argument(StringRef Key, unsigned N)
+DiagnosticInfoOptimizationCommonBase::Argument::Argument(StringRef Key,
+                                                         unsigned N)
     : Key(Key), Val(utostr(N)) {}
 
-void DiagnosticInfoOptimizationBase::print(DiagnosticPrinter &DP) const {
+void DiagnosticInfoOptimizationCommonBase::print(DiagnosticPrinter &DP) const {
   DP << getLocationStr() << ": " << getMsg();
   if (Hotness)
     DP << " (hotness: " << *Hotness << ")";
 }
 
 OptimizationRemark::OptimizationRemark(const char *PassName,
                                        StringRef RemarkName,
                                        const DebugLoc &DLoc, Value *CodeRegion)
void llvm::emitLoopInterleaveWarning(LLVMContext &Ctx, const Function &Fn,
   Ctx.diagnose(DiagnosticInfoOptimizationFailure(
       Fn, DLoc, Twine("loop not interleaved: " + Msg)));
 }
 
 void DiagnosticInfoISelFallback::print(DiagnosticPrinter &DP) const {
   DP << "Instruction selection used fallback path for " << getFunction();
 }
 
-DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
+DiagnosticInfoOptimizationCommonBase &DiagnosticInfoOptimizationCommonBase::
 operator<<(StringRef S) {
   Args.emplace_back(S);
   return *this;
 }
 
-DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
+DiagnosticInfoOptimizationCommonBase &DiagnosticInfoOptimizationCommonBase::
 operator<<(Argument A) {
   Args.push_back(std::move(A));
   return *this;
 }
 
-DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
+DiagnosticInfoOptimizationCommonBase &DiagnosticInfoOptimizationCommonBase::
 operator<<(setIsVerbose V) {
   IsVerbose = true;
   return *this;
 }
 
-DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
+DiagnosticInfoOptimizationCommonBase &DiagnosticInfoOptimizationCommonBase::
 operator<<(setExtraArgs EA) {
   FirstExtraArgIndex = Args.size();
   return *this;
 }
 
-std::string DiagnosticInfoOptimizationBase::getMsg() const {
+std::string DiagnosticInfoOptimizationCommonBase::getMsg() const {
   std::string Str;
   raw_string_ostream OS(Str);
-  for (const DiagnosticInfoOptimizationBase::Argument &Arg :
-       make_range(Args.begin(), FirstExtraArgIndex == -1
-                                    ? Args.end()
+  for (const DiagnosticInfoOptimizationCommonBase::Argument &Arg :
+       make_range(Args.begin(),
+                  FirstExtraArgIndex == -1 ? Args.end()
                                            : Args.begin() + FirstExtraArgIndex))
     OS << Arg.Val;
   return OS.str();
 }

lib/IR/LLVMContext.cpp

void LLVMContext::emitError(const Instruction *I, const Twine &ErrorStr) {
   diagnose(DiagnosticInfoInlineAsm(*I, ErrorStr));
 }
 
 static bool isDiagnosticEnabled(const DiagnosticInfo &DI) {
   // Optimization remarks are selective. They need to check whether the regexp
   // pattern, passed via one of the -pass-remarks* flags, matches the name of
   // the pass that is emitting the diagnostic. If there is no match, ignore the
   // diagnostic and return.
-  if (auto *Remark = dyn_cast<DiagnosticInfoOptimizationBase>(&DI))
+  if (auto *Remark = dyn_cast<DiagnosticInfoOptimizationCommonBase>(&DI))
     return Remark->isEnabled();
 
   return true;
 }
 
 const char *
 LLVMContext::getDiagnosticMessagePrefix(DiagnosticSeverity Severity) {
   switch (Severity) {