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

[Xinliang David Li via llvm-commits]

> MD data that should be copied over.
>
>
> I'm not sure how best to avoid bugs in the end.  However, users of LLVM are
> free to add arbitrary types of Metadata to Instructions.  It's not clear to
> me that they should all be copied.
>
>

In most of the cases, the copy happens between instructions of the
same type so new type of meta data should most likely be cloned as
well. In some cases, the meta data may need to be adjusted, but should
not be completely dropped.  Using the clone interface with an explicit
white list can end up with dropping new meta data silently.



>
> 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.
>
>
> Ok. For the sake of making this NFC, I added more code to deal with
> it. To make the default path (clone everything) fast, a new interface
> is added to strip all metadata except those specified in white list.
> Also added a FIXME there.
>
>
> Having to create an actually vector is a shame.  I'd expect to be able to
> send in {MD_dbg, MD_prof}.
>

Sure. My assumption was that the stripping should not be used
frequently and I disliked linear search more. I will make changes to
simplify the interface.


> Also, inserting and then removing Metadata is very expensive.  This blows up
> a SmallDenseMap (and it won't recover).

Ok, I will add support for copy white-listing.

thanks,

David

>
>
>
>
>   // 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?
>
>
>
> Now caller is calling the swapping explicitly.
>
>
> +  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?
>
>
>
> No. I think your suggestion is better.
>
> thanks,
>
> David
>
>
> +  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
>
>
>