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

[Joseph Tremoulet via llvm-commits]

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