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

[Duncan Exon Smith via llvm-commits]

> On Aug 17, 2016, at 14:29, Xinliang David Li <davidxl at google.com> wrote:
> 
> On Wed, Aug 17, 2016 at 1:19 PM, Duncan P. N. Exon Smith
> <dexonsmith at apple.com> wrote:
>> 
>>> 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?
> 
> Good question. I checked the original patch review thread, a reviewer
> asked white list without given reason.
> 
> It does not seem right -- for instance MD_predictable is another valid
> 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. 

> 
>> 
>> 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}. 

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

> 
>> 
>>> 
>>>   // 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
>>> 
>> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160818/8e30add2/attachment.html>