[PATCH] D23619: Refactor metadata copying/swapping code to make it sharable

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Aug-17, at 11:55, David Li <davidxl at google.com> wrote:
> 
> davidxl created this revision.
> davidxl added reviewers: spatel, dexonsmith.
> davidxl added a subscriber: llvm-commits.
> Herald added a subscriber: mzolotukhin.
> 
> No functional change is intended.
> 
> https://reviews.llvm.org/D23619
> 
> Files:
>  include/llvm/IR/Instruction.h
>  lib/IR/Instruction.cpp
>  lib/IR/Instructions.cpp
>  lib/Transforms/Scalar/LoopUnswitch.cpp
> 
> <D23619.68394.patch>

> Index: lib/Transforms/Scalar/LoopUnswitch.cpp
> ===================================================================
> --- lib/Transforms/Scalar/LoopUnswitch.cpp
> +++ lib/Transforms/Scalar/LoopUnswitch.cpp
> @@ -742,42 +742,6 @@
>    return &New;
>  }
>  
> -static void copyMetadata(Instruction *DstInst, const Instruction *SrcInst,
> -                         bool Swapped) {
> -  if (!SrcInst || !SrcInst->hasMetadata())
> -    return;
> -
> -  SmallVector<std::pair<unsigned, MDNode *>, 4> MDs;
> -  SrcInst->getAllMetadata(MDs);
> -  for (auto &MD : MDs) {
> -    switch (MD.first) {
> -    default:
> -      break;
> -    case LLVMContext::MD_prof:
> -      if (Swapped && MD.second->getNumOperands() == 3 &&
> -          isa<MDString>(MD.second->getOperand(0))) {
> -        MDString *MDName = cast<MDString>(MD.second->getOperand(0));
> -        if (MDName->getString() == "branch_weights") {
> -          auto *ValT = cast_or_null<ConstantAsMetadata>(
> -                           MD.second->getOperand(1))->getValue();
> -          auto *ValF = cast_or_null<ConstantAsMetadata>(
> -                           MD.second->getOperand(2))->getValue();
> -          assert(ValT && ValF && "Invalid Operands of branch_weights");
> -          auto NewMD =
> -              MDBuilder(DstInst->getParent()->getContext())
> -                  .createBranchWeights(cast<ConstantInt>(ValF)->getZExtValue(),
> -                                       cast<ConstantInt>(ValT)->getZExtValue());
> -          MD.second = NewMD;
> -        }
> -      }
> -      // fallthrough.
> -    case LLVMContext::MD_make_implicit:
> -    case LLVMContext::MD_dbg:
> -      DstInst->setMetadata(MD.first, MD.second);
> -    }
> -  }
> -}
> -
>  /// Emit a conditional branch on two values if LIC == Val, branch to TrueDst,
>  /// otherwise branch to FalseDest. Insert the code immediately before InsertPt.
>  void LoopUnswitch::EmitPreheaderBranchOnCondition(Value *LIC, Constant *Val,
> @@ -800,7 +764,8 @@
>  
>    // Insert the new branch.
>    BranchInst *BI = BranchInst::Create(TrueDest, FalseDest, BranchVal, InsertPt);
> -  copyMetadata(BI, TI, Swapped);
> +  if (TI)
> +    BI->copyMetadata(*TI, Swapped);

This has different semantics than the old code.

  - Previously, only MD_prof (with a possible swap), MD_dbg and
    MD_make_implicit were copied.

  - Now arbitrary metadata is copied.

Are you sure this makes sense?  Why?

Another possibility that would let you keep the current semantics:
copyMetadata could take a list of IDs ( ArrayRef<unsigned>, say) for the
metadata that should be copied over.

>  
>    // If either edge is critical, split it. This helps preserve LoopSimplify
>    // form for enclosing loops.
> Index: lib/IR/Instructions.cpp
> ===================================================================
> --- lib/IR/Instructions.cpp
> +++ lib/IR/Instructions.cpp
> @@ -1209,15 +1209,7 @@
>  
>    // Update profile metadata if present and it matches our structural
>    // expectations.
> -  MDNode *ProfileData = getMetadata(LLVMContext::MD_prof);
> -  if (!ProfileData || ProfileData->getNumOperands() != 3)
> -    return;
> -
> -  // The first operand is the name. Fetch them backwards and build a new one.
> -  Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2),
> -                     ProfileData->getOperand(1)};
> -  setMetadata(LLVMContext::MD_prof,
> -              MDNode::get(ProfileData->getContext(), Ops));
> +  swapProfMetadata();

It looks like swapProfMetadata() could be done separately.

>  }
>  
>  BasicBlock *BranchInst::getSuccessorV(unsigned idx) const {
> Index: lib/IR/Instruction.cpp
> ===================================================================
> --- lib/IR/Instruction.cpp
> +++ lib/IR/Instruction.cpp
> @@ -627,6 +627,41 @@
>    llvm_unreachable("Subclass of Instruction failed to implement cloneImpl");
>  }
>  
> +void Instruction::swapProfMetadata() {
> +  MDNode *ProfileData = getMetadata(LLVMContext::MD_prof);
> +  if (!ProfileData || ProfileData->getNumOperands() != 3 ||
> +      !isa<MDString>(ProfileData->getOperand(0)))
> +    return;
> +
> +  MDString *MDName = cast<MDString>(ProfileData->getOperand(0));
> +  if (MDName->getString() != "branch_weights")
> +    return;
> +
> +  // The first operand is the name. Fetch them backwards and build a new one.
> +  Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2),
> +                     ProfileData->getOperand(1)};
> +  setMetadata(LLVMContext::MD_prof,
> +              MDNode::get(ProfileData->getContext(), Ops));
> +}
> +
> +void Instruction::copyMetadata(const Instruction &SrcInst, bool doSwapping) {

"ShouldSwap" seems like a better name for the "doSwapping" parameter.
However, I wonder whether it makes sense at all here?  Why not just have
the caller call swapProfMetadata?

> +  if (!SrcInst.hasMetadata())
> +    return;
> +
> +  // Otherwise, enumerate and copy over metadata from the old instruction to the
> +  // new one.
> +  SmallVector<std::pair<unsigned, MDNode *>, 4> TheMDs;
> +  SrcInst.getAllMetadataOtherThanDebugLoc(TheMDs);
> +  for (const auto &MD : TheMDs)
> +    setMetadata(MD.first, MD.second);
> +
> +  setDebugLoc(SrcInst.getDebugLoc());
> +
> +  if (doSwapping)
> +    swapProfMetadata();

I.e., the caller could do this.  Or are you planning to optimize this
in the future?

> +  return;
> +}
> +
>  Instruction *Instruction::clone() const {
>    Instruction *New = nullptr;
>    switch (getOpcode()) {
> @@ -641,16 +676,6 @@
>    }
>  
>    New->SubclassOptionalData = SubclassOptionalData;
> -  if (!hasMetadata())
> -    return New;
> -
> -  // Otherwise, enumerate and copy over metadata from the old instruction to the
> -  // new one.
> -  SmallVector<std::pair<unsigned, MDNode *>, 4> TheMDs;
> -  getAllMetadataOtherThanDebugLoc(TheMDs);
> -  for (const auto &MD : TheMDs)
> -    New->setMetadata(MD.first, MD.second);
> -
> -  New->setDebugLoc(getDebugLoc());
> +  New->copyMetadata(*this, false);
>    return New;
>  }
> Index: include/llvm/IR/Instruction.h
> ===================================================================
> --- include/llvm/IR/Instruction.h
> +++ include/llvm/IR/Instruction.h
> @@ -197,6 +197,15 @@
>    void setMetadata(unsigned KindID, MDNode *Node);
>    void setMetadata(StringRef Kind, MDNode *Node);
>  
> +  /// Copy metadata from \p SrcInst to this instruction. If \p doSwapping
> +  /// is set to true, \c MD_prof meta data will be swapped.
> +  void copyMetadata(const Instruction &SrcInst, bool doSwapping);

s/doSwapping/ShouldSwapProf/ (if we keep it).

> +
> +  /// If the instruction has "branch_weights" MD_prof metadata and the MDNode
> +  /// has three operands (including name string), swap the order of the
> +  /// metadata.
> +  void swapProfMetadata();
> +
>    /// Drop all unknown metadata except for debug locations.
>    /// @{
>    /// Passes are required to drop metadata they don't understand. This is a
>