[PATCH] D12456: [IR] Add operand bundles to CallInst and InvokeInst.
Joseph Tremoulet via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 31 13:17:56 PDT 2015
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
More information about the llvm-commits
mailing list