[PATCH] D12456: [IR] Add operand bundles to CallInst and InvokeInst.
Sanjoy Das via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 9 19:04:06 PDT 2015
> 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
>> + auto I = BundleTagCache.find(Tag);
>> + if (I != BundleTagCache.end())
>> + return&*I;
>> + auto *Entry =
>> + StringMapEntry<uint32_t>::Create(Tag,
>> + 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
> 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.
More information about the llvm-commits