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

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.

================
Comment at: include/llvm/IR/Function.h:67
@@ +66,3 @@
+   * bit 2    : HasPrologueData
+   * bit 3-14 : CallingConvention
+   * bit 15   : HasPersonalityFn
----------------
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).

================
Comment at: include/llvm/IR/Function.h:158
@@ -163,3 +157,3 @@
   void setCallingConv(CallingConv::ID CC) {
-    setValueSubclassData((getSubclassDataFromValue() & 7) |
-                         (static_cast<unsigned>(CC) << 3));
+    setValueSubclassData(getSubclassDataFromValue() |
+                         ((static_cast<unsigned>(CC) & 0xfff) << 3));
----------------
I'm not sure this is correct -- are you `|` ing over the new calling convention over what was there previously?  Won't setting the calling convention to `4` and then to `2` end up setting it to `6`?

================
Comment at: include/llvm/IR/Function.h:488
@@ -493,1 +487,3 @@
 
+  bool hasPersonalityFn() const {
+    return getSubclassDataFromValue() & (1<<15);
----------------
Add a doxygen comment.

================
Comment at: lib/IR/Function.cpp:973
@@ +972,3 @@
+  allocHungoffUselist();
+  auto *CPN = ConstantPointerNull::get(Type::getInt1PtrTy(getContext(), 0));
+  Op<Idx>().set(C ? C : CPN);
----------------
Minor: I'd call `ConstantPointerNull::get` only if `C` is `nullptr`.

Also, do you really need to support the `setHungoffOperand(nullptr)` case?  I think creating and passing in a `i8* null` or whatever should be up to the caller.


http://reviews.llvm.org/D13829