[PATCH] D13120: [Function] Clean up {prefix, prologue} data routines (NFC)

[Justin Bogner via llvm-commits]

Vedant Kumar via llvm-commits <llvm-commits at lists.llvm.org> writes:
> vsk created this revision.
> vsk added a reviewer: pcc.
> vsk added a subscriber: llvm-commits.
>
> Factor out some common code used to get+set function prefix/prologue
> data. This may come in handy if we ever decide to store personality
> functions in the same way we store prefix/prologue data.
>
> Also, update the number of bits to reflect how many we actually use to
> store calling convention identifiers.

LGTM with a couple of minor comments.

> http://reviews.llvm.org/D13120
>
> Files:
>   include/llvm/IR/Function.h
>   lib/IR/Function.cpp
>   lib/IR/LLVMContextImpl.h
>
> Index: lib/IR/LLVMContextImpl.h
> ===================================================================
> --- lib/IR/LLVMContextImpl.h
> +++ lib/IR/LLVMContextImpl.h
> @@ -972,16 +972,16 @@
>    /// instructions in different blocks at the same location.
>    DenseMap<std::pair<const char *, unsigned>, unsigned> DiscriminatorTable;
>  
> +  typedef DenseMap<const Function *, ReturnInst *> FunctionDataMapTy;
> +
>    /// \brief Mapping from a function to its prefix data, which is stored as the
>    /// operand of an unparented ReturnInst so that the prefix data has a Use.
> -  typedef DenseMap<const Function *, ReturnInst *> PrefixDataMapTy;
> -  PrefixDataMapTy PrefixDataMap;
> +  FunctionDataMapTy PrefixDataMap;
>  
>    /// \brief Mapping from a function to its prologue data, which is stored as
>    /// the operand of an unparented ReturnInst so that the prologue data has a
>    /// Use.
> -  typedef DenseMap<const Function *, ReturnInst *> PrologueDataMapTy;
> -  PrologueDataMapTy PrologueDataMap;
> +  FunctionDataMapTy PrologueDataMap;
>  
>    int getOrAddScopeRecordIdxEntry(MDNode *N, int ExistingIdx);
>    int getOrAddScopeInlinedAtIdxEntry(MDNode *Scope, MDNode *IA,int ExistingIdx);
> Index: lib/IR/Function.cpp
> ===================================================================
> --- lib/IR/Function.cpp
> +++ lib/IR/Function.cpp
> @@ -929,62 +929,59 @@
>    return false;
>  }
>  
> +static Constant *
> +getFunctionData(const Function *F,
> +                const LLVMContextImpl::FunctionDataMapTy &Map) {
> +  const auto &Entry = Map.find(F);
> +  assert(Entry != Map.end());
> +  return cast<Constant>(Entry->second->getReturnValue());
> +}
> +
> +static unsigned setFunctionData(Function *F,
> +                                LLVMContextImpl::FunctionDataMapTy &Map,
> +                                Constant *Data, unsigned SCData, unsigned Bit) {

Might be worth including a doc comment.

> +  ReturnInst *&Holder = Map[F];
> +  if (Data) {
> +    if (Holder)
> +      Holder->setOperand(0, Data);
> +    else
> +      Holder = ReturnInst::Create(F->getContext(), Data);
> +    return SCData | (1 << Bit);
> +  } else {
> +    delete Holder;
> +    Map.erase(F);
> +    return SCData & ~(1 << Bit);
> +  }
> +}
> +
>  Constant *Function::getPrefixData() const {
>    assert(hasPrefixData());
> -  const LLVMContextImpl::PrefixDataMapTy &PDMap =
> -      getContext().pImpl->PrefixDataMap;
> -  assert(PDMap.find(this) != PDMap.end());
> -  return cast<Constant>(PDMap.find(this)->second->getReturnValue());
> +  return getFunctionData(this, getContext().pImpl->PrefixDataMap);
>  }
>  
>  void Function::setPrefixData(Constant *PrefixData) {
>    if (!PrefixData && !hasPrefixData())
>      return;
>  
>    unsigned SCData = getSubclassDataFromValue();
> -  LLVMContextImpl::PrefixDataMapTy &PDMap = getContext().pImpl->PrefixDataMap;
> -  ReturnInst *&PDHolder = PDMap[this];
> -  if (PrefixData) {
> -    if (PDHolder)
> -      PDHolder->setOperand(0, PrefixData);
> -    else
> -      PDHolder = ReturnInst::Create(getContext(), PrefixData);
> -    SCData |= (1<<1);
> -  } else {
> -    delete PDHolder;
> -    PDMap.erase(this);
> -    SCData &= ~(1<<1);
> -  }
> +  SCData = setFunctionData(this, getContext().pImpl->PrefixDataMap, PrefixData,
> +                           SCData, 1);

Please comment the Bit argument (ie, `/*Bit=*/1` or so)

>    setValueSubclassData(SCData);
>  }
>  
>  Constant *Function::getPrologueData() const {
>    assert(hasPrologueData());
> -  const LLVMContextImpl::PrologueDataMapTy &SOMap =
> -      getContext().pImpl->PrologueDataMap;
> -  assert(SOMap.find(this) != SOMap.end());
> -  return cast<Constant>(SOMap.find(this)->second->getReturnValue());
> +  return getFunctionData(this, getContext().pImpl->PrologueDataMap);
>  }
>  
>  void Function::setPrologueData(Constant *PrologueData) {
>    if (!PrologueData && !hasPrologueData())
>      return;
>  
> -  unsigned PDData = getSubclassDataFromValue();
> -  LLVMContextImpl::PrologueDataMapTy &PDMap = getContext().pImpl->PrologueDataMap;
> -  ReturnInst *&PDHolder = PDMap[this];
> -  if (PrologueData) {
> -    if (PDHolder)
> -      PDHolder->setOperand(0, PrologueData);
> -    else
> -      PDHolder = ReturnInst::Create(getContext(), PrologueData);
> -    PDData |= (1<<2);
> -  } else {
> -    delete PDHolder;
> -    PDMap.erase(this);
> -    PDData &= ~(1<<2);
> -  }
> -  setValueSubclassData(PDData);
> +  unsigned SCData = getSubclassDataFromValue();
> +  SCData = setFunctionData(this, getContext().pImpl->PrologueDataMap,
> +                           PrologueData, SCData, 2);
> +  setValueSubclassData(SCData);
>  }
>  
>  void Function::setEntryCount(uint64_t Count) {
> Index: include/llvm/IR/Function.h
> ===================================================================
> --- include/llvm/IR/Function.h
> +++ include/llvm/IR/Function.h
> @@ -78,7 +78,7 @@
>     * bit 0  : HasLazyArguments
>     * bit 1  : HasPrefixData
>     * bit 2  : HasPrologueData
> -   * bit 3-6: CallingConvention
> +   * bit 3-9: CallingConvention
>     */
>  
>    /// Bits from GlobalObject::GlobalObjectSubclassData.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits