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

[Sanjoy Das via llvm-commits]

 >
 > 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! :) ).

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

-- Sanjoy