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

[Sanjoy Das via llvm-commits]

I don't know why this hasn't showed up on llvm-commits yet, but I've
replied to the review on phabricator.

On Mon, Aug 31, 2015 at 1:17 PM, Joseph Tremoulet <jotrem at microsoft.com> wrote:
> JosephTremoulet added inline comments.
>
> ================
> Comment at: include/llvm/IR/InstrTypes.h:1107
> @@ +1106,3 @@
> +  OperandBundleSetT() {}
> +  explicit OperandBundleSetT(StringRef Tag, ArrayRef<InputTy> Inputs)
> +      : Tag(Tag), Inputs(Inputs) {}
> ----------------
> nit: Why `explicit`, when we have two parameters?
>
> ================
> Comment at: include/llvm/IR/InstrTypes.h:1130
> @@ +1129,3 @@
> +    auto *Begin = bundle_op_info_begin();
> +    auto *InclusiveEnd = bundle_op_info_end() - 1;
> +
> ----------------
> super-nit: I think `Back` would be a more common name for this than `InclusiveEnd`
>
> ================
> Comment at: include/llvm/IR/Instructions.h:1429
> @@ +1428,3 @@
> +        unsigned(Args.size()) + CountBundleInputs(Bundles) + 1;
> +    const unsigned DescriptorBytes = Bundles.size() * sizeof(BundleOpInfo);
> +
> ----------------
> Should we pad this to pointer-align the `StringMapEntry<uint32_t> *Tag` field of the `BundleOpInfo`s (and likewise for the other `CallInst::Create` overload and the two `InvokeInst::Create`s)?
>
> ================
> Comment at: lib/IR/LLVMContextImpl.h:989-990
> @@ -988,1 +988,4 @@
>
> +  StringMap<uint32_t> BundleTagCache;
> +  StringMapEntry<uint32_t> *internBundleTag(StringRef Tag);
> +
> ----------------
> nit: worth a comment?
>
> ================
> Comment at: lib/IR/User.cpp:121-122
> @@ -119,1 +120,4 @@
>        DescBytes == 0 ? 0 : (DescBytes + sizeof(DescriptorInfo));
> +  assert(DescBytesToAllocate % 4 == 0 &&
> +         "We need this to satisfy alignment constraints for Uses");
> +
> ----------------
> Ok, I should have read ahead :).  But I still think it would be good to discuss alignment in the method header comment, and to 8-byte align the CallInst/InvokeInst bundles on 64-bit platforms for the pointer to the string map entry.
>
>
> http://reviews.llvm.org/D12456
>
>
>



-- 
Sanjoy Das
http://playingwithpointers.com