[PATCH] D13829: WIP: clean up the way optional Function data is managed

[Vedant Kumar via llvm-commits]

vsk marked 4 inline comments as done.
vsk added a comment.

Addressed some inline comments.


================
Comment at: include/llvm/IR/Function.h:67
@@ +66,3 @@
+   * bit 2    : HasPrologueData
+   * bit 3-14 : CallingConvention
+   * bit 15   : HasPersonalityFn
----------------
sanjoy wrote:
> Was the earlier comment incorrect? (i.e. `CallingConvention` should have been `3-15` earlier)?
> 
> If so, you might want to correct that mistake separately and check that in, and rebase this patch over that.  That'll make the diff clearer.
> 
> Also, why did you choose bit 15?  Doesn't it make sense to group this together with the rest of the `Has*` enums, before `CallingConvention`?  (What you have here is fine btw, I'm just curious).
Yes, `CallingConvention` should have been `3-15` earlier. I will correct it in D13826.

I chose bit 15 because of an incorrect assumption. I thought the SubclassData field is serialized, and that we needed to be sensitive about re-ordering the bits in it. I'll switch over to bit 4 in D13826.

================
Comment at: lib/IR/Function.cpp:973
@@ +972,3 @@
+  allocHungoffUselist();
+  Op<Idx>().set(
+      C ? C : ConstantPointerNull::get(Type::getInt1PtrTy(getContext(), 0)));
----------------
Fixed.

Yes, we have to remap `setHungoffOperand(nullptr)` to a real Constant. llvm assumes that operands are non-null all over the place, in ways that can't be guarded against by  checking `hasPersonality`, `hasPrefixData` etc. The previous API got around this problem by deleting its operand when nullptr is set.


http://reviews.llvm.org/D13829