[PATCH] D12456: [IR] Add operand bundles to CallInst and InvokeInst.

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

> On 2015-Sep-08, at 21:54, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> 
> sanjoy updated this revision to Diff 34298.
> sanjoy added a comment.
> 
> - add more comments to OperandBundleUser
> - move Use& population logic to populateBundleOperandInfos to keep everything in one place.
> 
> 
> http://reviews.llvm.org/D12456
> 
> Files:
>  include/llvm/IR/CallSite.h
>  include/llvm/IR/InstrTypes.h
>  include/llvm/IR/Instructions.h
>  lib/IR/Instructions.cpp
>  lib/IR/LLVMContextImpl.cpp
>  lib/IR/LLVMContextImpl.h
> 
> <D12456.34298.patch>

Not sure if you want to wait for other reviews, but FWIW this LGTM with
a few nitpicks below.

> Index: lib/IR/LLVMContextImpl.cpp
> ===================================================================
> --- lib/IR/LLVMContextImpl.cpp
> +++ lib/IR/LLVMContextImpl.cpp
> @@ -219,6 +219,19 @@
>    return hash_combine_range(Ops.begin(), Ops.end());
>  }
>  
> +StringMapEntry<uint32_t> *LLVMContextImpl::internBundleTag(StringRef Tag) {

I don't really understand this function name.  What does "intern" mean
in this context?  Is it a verb?

(From its functionality, I'd expect a name similar to
"getOrInsertBundleTag()".)

> +  auto I = BundleTagCache.find(Tag);
> +  if (I != BundleTagCache.end())
> +    return &*I;
> +
> +  auto *Entry =
> +      StringMapEntry<uint32_t>::Create(Tag, BundleTagCache.getAllocator(), 0);
> +  bool WasInserted = BundleTagCache.insert(Entry);
> +  (void)WasInserted;
> +  assert(WasInserted && "Expected entry to be inserted");
> +  return Entry;

Is this function doing anything different from the following one-liner?
--
return *&BundleTagCache.insert(std::make_pair(Tag, 0)).first;
--

> +}
> +
>  // ConstantsContext anchors
>  void UnaryConstantExpr::anchor() { }
>  
> Index: include/llvm/IR/CallSite.h
> ===================================================================
> --- include/llvm/IR/CallSite.h
> +++ include/llvm/IR/CallSite.h
> @@ -379,10 +396,15 @@
>  
>  private:
>    unsigned getArgumentEndOffset() const {
> -    if (isCall())
> -      return 1; // Skip Callee
> -    else
> -      return 3; // Skip BB, BB, Callee
> +    if (isCall()) {
> +      // Skip [ operand bundles ], Callee
> +      auto *CI = cast<CallInst>(getInstruction());

I haven't looked at how `isCall()` is implemented, but this looks
vaguely like an `isa<>` (in disguise) plus `cast<>`.  If so, this would
be better as:
--
Instruction *I = getInstruction();
if (auto *CI = dyn_cast<CallInst>(I))
  return 1 + CI->...;
return 3 + cast<InvokeInst>(I)->...;
--

> +      return 1 + CI->getNumTotalBundleOperands();
> +    } else {
> +      // Skip [ operand bundles ], BB, BB, Callee
> +      auto *II = cast<InvokeInst>(getInstruction());
> +      return 3 + II->getNumTotalBundleOperands();
> +    }
>    }
>  
>    IterTy getCallee() const {