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

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

> On 2015-Sep-09, at 19:04, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
> 
> >
> > Not sure if you want to wait for other reviews, but FWIW this LGTM with
> > a few nitpicks below.
> 
> I'd prefer landing this one with http://reviews.llvm.org/D12457 as I
> don't want to check in a bunch of untested code.  I'm currently
> waiting for reviews on D12457 (hint! :) ).

Done :).

> > 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()".)
> 
> Done.
> 
> >> +  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;
> 
> Nothing except the assertion on `WasInserted`, but that looks
> redundant.  Fixed.
> 
> > 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)->...;
> > --
> 
> `CallSite` uses the lower bits of the `Instruction *` to remember if
> the `Instruction` is a `CallInst` or an `InvokeInst`.  So, at least in
> theory using `isCall` over `isa<CallInst>` saves one dereference; but
> that's of dubious value since after this change `getArgumentEndOffset`
> will have to touch memory anyway in `getNumTotalBundleOperands`.
> 
> I have a mild preference for using `isCall` since that's what the rest
> of `CallSite` uses.

Okay, sounds good.  I didn't know CallSite had this cached.