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

[Xinliang David Li via llvm-commits]

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.

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


>
>>
>>    // 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
>>
>