[PATCH] LowerBitSets: Use byte arrays instead of bit sets to represent in-memory bit sets.

[JF Bastien]

================
Comment at: include/llvm/Transforms/IPO/LowerBitSets.h:174
@@ +173,3 @@
+  /// The number of bytes allocated so far for each of the bits.
+  uint64_t BitAllocs[8];
+
----------------
pcc wrote:
> jfb wrote:
> > Could you add a symbolic constant for 8? Also, why 8? Is it good for x86 but now as good for ARM?
> Are you asking why I am using 8 bits per array element? Well, for one thing it gives us better packing (the more bins there are, the less evenly they will be filled). For another, the instruction sequences can be slightly shorter, both on x86 and ARM. For example, this code:
> 
> ```
> typedef unsigned long T;
> typedef void (*F)(void);
> T table;
> 
> unsigned char bitset[1000];
> void FOO(T **obj) {
>   T *vptr = *obj;
>   T ptr_offset = (T)vptr - (T)table;
>   T t1 = ptr_offset << (sizeof(void*)*8 - 3);
>   T t2 = ptr_offset >> 3;
>   T t3 = t1 | t2;
>   unsigned bits = bitset[t3];
>   if ((t3 >= TABLE_SIZE) || (bits & 1))
>     __builtin_trap();
>   F f = (F)vptr[4];
>   f();
> }
> ```
> 
> is one byte shorter (on x86) or one instruction shorter (on ARM) than this code:
> 
> ```
> typedef unsigned long T;
> typedef void (*F)(void);
> T table;
> 
> unsigned short bitset[1000];
> void FOO(T **obj) {
>   T *vptr = *obj;
>   T ptr_offset = (T)vptr - (T)table;
>   T t1 = ptr_offset << (sizeof(void*)*8 - 3);
>   T t2 = ptr_offset >> 3;
>   T t3 = t1 | t2;
>   unsigned bits = bitset[t3];
>   if ((t3 >= TABLE_SIZE) || (bits & 0x100))
>     __builtin_trap();
>   F f = (F)vptr[4];
>   f();
> }
> ```
> I guess I could parameterise this but it seems like it would add more complexity than we need. Happy to document the choice of 8 bits per array element.
Yes, documentation for the choice of 8 is good :)

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:360
@@ -268,1 +359,3 @@
+                                      ByteArrayInfo *&BAI, Value *BitOffset) {
+  if (BSI.BitSize <= 64) {
     // If the bit set is sufficiently small, we can avoid a load by bit testing
----------------
pcc wrote:
> jfb wrote:
> > Just to be sure I understand: this inlines small (<= 64 bit) bitsets. The current patch only benefits from co-locating larger bitsets, right? It would probably be good for the docs to specify this double approach to reducing bitset footprint.
> We already document the technique for inlining <=64 bits:
> 
> http://clang.llvm.org/docs/ControlFlowIntegrityDesign.html#short-inline-bit-vectors
> 
> and I do plan to document byte arrays on that page once this lands.
sgtm

http://reviews.llvm.org/D7954

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/