[GVN, OptDiag] Include the value that is forwarded in load elimination

[anemet]

Summary

This requires some changes to the opt-diag API. Hal and I have
discussed this at the Dev Meeting and came up with a streaming delimiter
(setExtraArgs) to solve this.

Arguments after this delimiter are only included in the optimization
records and not in the remarks printed in the compiler output. (Note,
how in the test the content of the YAML file changes but the remarks on
the compiler output don't.)

This implements the green GVN message with a bug fix at line
http://lab.llvm.org:8080/artifacts/opt-view_test-suite/build/SingleSource/Benchmarks/Dhrystone/CMakeFiles/dry.dir/html/_org_test-suite_SingleSource_Benchmarks_Dhrystone_dry.c.html#L446

The fix is that now we properly include the constant value in the
message: "load of type i32 eliminated in favor of 7"

[anemet]

Rebase patch-set

[hfinkel]

LGTM

lib/IR/DiagnosticInfo.cpp

190

In the future, we should try harder here to use debug info to get a real name.

[anemet]

lib/IR/DiagnosticInfo.cpp

190

Added a FIXME about this.

Thanks for your review!

include/llvm/Analysis/OptimizationDiagnosticInfo.h

 };
 
 /// \brief Add a small namespace to avoid name clashes with the classes used in
 /// the streaming interface.  We want these to be short for better
 /// write/readability.
 namespace ore {
 using NV = DiagnosticInfoOptimizationBase::Argument;
 using setIsVerbose = DiagnosticInfoOptimizationBase::setIsVerbose;
+using setExtraArgs = DiagnosticInfoOptimizationBase::setExtraArgs;
 }
 
 /// OptimizationRemarkEmitter legacy analysis pass
 ///
 /// Note that this pass shouldn't generally be marked as preserved by other
 /// passes.  It's holding onto BFI, so if the pass does not preserve BFI, BFI
 /// could be freed.
 class OptimizationRemarkEmitterWrapperPass : public FunctionPass {

include/llvm/IR/DiagnosticInfo.h

 };
 
 /// Common features for diagnostics dealing with optimization remarks.
 class DiagnosticInfoOptimizationBase : 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 setExtraArgs {};
+
   /// \brief Used in the streaming interface as the general argument type.  It
   /// internally converts everything into a key-value pair.
   struct Argument {
     StringRef Key;
     std::string Val;
     // If set, the debug location corresponding to the value.
     DebugLoc DLoc;
 
DiagnosticInfoOptimizationBase(enum DiagnosticKind Kind,
       : DiagnosticInfoWithDebugLocBase(Kind, Severity, Fn, DLoc),
         PassName(PassName), Hotness(Hotness) {
     Args.push_back(Argument(Msg.str()));
   }
 
   DiagnosticInfoOptimizationBase &operator<<(StringRef S);
   DiagnosticInfoOptimizationBase &operator<<(Argument A);
   DiagnosticInfoOptimizationBase &operator<<(setIsVerbose V);
+  DiagnosticInfoOptimizationBase &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
private:
   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 *>;
 };
 
 /// 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

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)
-    : Key(Key), Val(GlobalValue::getRealLinkageName(V->getName())) {
+    : 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.
+  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)
     : Key(Key) {
   raw_string_ostream OS(Val);
   OS << *T;
 }
 

 }
 
 DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
 operator<<(setIsVerbose V) {
   IsVerbose = true;
   return *this;
 }
 
+DiagnosticInfoOptimizationBase &DiagnosticInfoOptimizationBase::
+operator<<(setExtraArgs EA) {
+  FirstExtraArgIndex = Args.size();
+  return *this;
+}
+
 std::string DiagnosticInfoOptimizationBase::getMsg() const {
   std::string Str;
   raw_string_ostream OS(Str);
-  for (const DiagnosticInfoOptimizationBase::Argument &Arg : Args)
+  for (const DiagnosticInfoOptimizationBase::Argument &Arg :
+       make_range(Args.begin(), FirstExtraArgIndex == -1
+                                    ? Args.end()
+                                    : Args.begin() + FirstExtraArgIndex))
     OS << Arg.Val;
   return OS.str();
 }

lib/Transforms/Scalar/GVN.cpp

if (isa<PHINode>(V))
       V->takeName(LI);
     if (Instruction *I = dyn_cast<Instruction>(V))
       if (LI->getDebugLoc())
         I->setDebugLoc(LI->getDebugLoc());
     if (V->getType()->getScalarType()->isPointerTy())
       MD->invalidateCachedPointerInfo(V);
     markInstructionForDeletion(LI);
     ++NumGVNLoad;
+    using namespace ore;
     ORE->emit(OptimizationRemark(DEBUG_TYPE, "LoadElim", LI)
-              << "load of type " << ore::NV("Type", LI->getType())
-              << " eliminated");
+              << "load of type " << NV("Type", LI->getType())
+              << " eliminated" << setExtraArgs()
+              << " in favor of " << NV("InfavorOfValue", V));
     return true;
   }
 
   // Step 4: Eliminate partial redundancy.
   if (!EnablePRE || !EnableLoadPRE)
     return false;
 
   return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks);
if (AnalyzeLoadAvailability(L, Dep, L->getPointerOperand(), AV)) {
     Value *AvailableValue = AV.MaterializeAdjustedValue(L, L, *this);
 
     // Replace the load!
     patchAndReplaceAllUsesWith(L, AvailableValue);
     markInstructionForDeletion(L);
     ++NumGVNLoad;
     DEBUG(
         dbgs() << "GVN: load eliminated: " << *L << "\n");
+    using namespace ore;
     ORE->emit(OptimizationRemark(DEBUG_TYPE, "LoadElim", L)
-              << "load of type " << ore::NV("Type", L->getType())
-              << " eliminated");
+              << "load of type " << NV("Type", L->getType()) << " eliminated"
+              << setExtraArgs() << " in favor of "
+              << NV("InfavorOfValue", AvailableValue));
     // Tell MDA to rexamine the reused pointer since we might have more
     // information after forwarding it.
     if (MD && AvailableValue->getType()->getScalarType()->isPointerTy())
       MD->invalidateCachedPointerInfo(AvailableValue);
     return true;
   }
 
   return false;

test/Transforms/GVN/opt-remarks.ll

 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            gvn
 ; YAML-NEXT: Name:            LoadElim
 ; YAML-NEXT: Function:        arg
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'load of type '
 ; YAML-NEXT:   - Type:            i32
 ; YAML-NEXT:   - String:          ' eliminated'
+; YAML-NEXT:   - String:          ' in favor of '
+; YAML-NEXT:   - InfavorOfValue:  i
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Passed
 ; YAML-NEXT: Pass:            gvn
 ; YAML-NEXT: Name:            LoadElim
 ; YAML-NEXT: Function:        const
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'load of type '
 ; YAML-NEXT:   - Type:            i32
 ; YAML-NEXT:   - String:          ' eliminated'
+; YAML-NEXT:   - String:          ' in favor of '
+; YAML-NEXT:   - InfavorOfValue:  '4'
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Passed
 ; YAML-NEXT: Pass:            gvn
 ; YAML-NEXT: Name:            LoadElim
 ; YAML-NEXT: Function:        inst
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'load of type '
 ; YAML-NEXT:   - Type:            i32
 ; YAML-NEXT:   - String:          ' eliminated'
+; YAML-NEXT:   - String:          ' in favor of '
+; YAML-NEXT:   - InfavorOfValue:  load
 ; YAML-NEXT: ...
 
 
 define i32 @arg(i32* %p, i32 %i) {
 entry:
   store i32 %i, i32* %p
   %load = load i32, i32* %p
   ret i32 %load