[PATCH] D13829: [IR] Move optional data in llvm::Function into a hungoff uselist

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

> On 2015-Dec-02, at 12:45, Vedant Kumar <vsk at apple.com> wrote:
> 
> vsk added a comment.
> 
> This isn't urgent but it's been a while. Ping?
> 
> 
> http://reviews.llvm.org/D13829

> Index: lib/IR/Function.cpp
> ===================================================================
> --- lib/IR/Function.cpp
> +++ lib/IR/Function.cpp
> @@ -929,61 +920,67 @@
>    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());
> -}
> -
> -/// setFunctionData - Set "Map[F] = Data". Return an updated SubclassData value
> -/// in which Bit is low iff Data is null.
> -static unsigned setFunctionData(Function *F,
> -                                LLVMContextImpl::FunctionDataMapTy &Map,
> -                                Constant *Data, unsigned SCData, unsigned Bit) {
> -  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::getPersonalityFn() const {
> +  assert(hasPersonalityFn() && getNumOperands());
> +  return cast<Constant>(Op<0>());
> +}
> +
> +void Function::setPersonalityFn(Constant *Fn) {
> +  if (Fn)
> +    setHungoffOperand<0>(Fn);
> +  setValueSubclassDataBit(3, Fn != nullptr);
>  }
>  
>  Constant *Function::getPrefixData() const {
> -  assert(hasPrefixData());
> -  return getFunctionData(this, getContext().pImpl->PrefixDataMap);
> +  assert(hasPrefixData() && getNumOperands());
> +  return cast<Constant>(Op<1>());
>  }
>  
>  void Function::setPrefixData(Constant *PrefixData) {
> -  if (!PrefixData && !hasPrefixData())
> -    return;
> -
> -  unsigned SCData = getSubclassDataFromValue();
> -  SCData = setFunctionData(this, getContext().pImpl->PrefixDataMap, PrefixData,
> -                           SCData, /*Bit=*/1);
> -  setValueSubclassData(SCData);
> +  if (PrefixData)
> +    setHungoffOperand<1>(PrefixData);
> +  setValueSubclassDataBit(1, PrefixData != nullptr);
>  }
>  
>  Constant *Function::getPrologueData() const {
> -  assert(hasPrologueData());
> -  return getFunctionData(this, getContext().pImpl->PrologueDataMap);
> +  assert(hasPrologueData() && getNumOperands());
> +  return cast<Constant>(Op<2>());
>  }
>  
>  void Function::setPrologueData(Constant *PrologueData) {
> -  if (!PrologueData && !hasPrologueData())
> +  if (PrologueData)
> +    setHungoffOperand<2>(PrologueData);
> +  setValueSubclassDataBit(2, PrologueData != nullptr);
> +}
> +
> +void Function::allocHungoffUselist() {
> +  // If we've already allocated a uselist, stop here.
> +  if (getNumOperands())
>      return;
>  
> -  unsigned SCData = getSubclassDataFromValue();
> -  SCData = setFunctionData(this, getContext().pImpl->PrologueDataMap,
> -                           PrologueData, SCData, /*Bit=*/2);
> -  setValueSubclassData(SCData);
> +  allocHungoffUses(3, /*IsPhi=*/ false);
> +  setNumHungOffUseOperands(3);
> +
> +  // Initialize the uselist with placeholder operands to allow traversal.
> +  auto *CPN = ConstantPointerNull::get(Type::getInt1PtrTy(getContext(), 0));
> +  Op<0>().set(CPN);
> +  Op<1>().set(CPN);
> +  Op<2>().set(CPN);

Somehow using `ConstantPointerNull`s here seems really strange to me.
I don't think we should be adding uses of `ConstantPointerNull` just
as a placeholder.

On the other hand, I'm not sure I have a better suggestion.  Can
someone else give their opinion?  Is this fine?

(If you can get me shouted down at that point, then this patch LGTM.)

> +}