[PATCH] Introduce bitset metadata format and bitset lowering pass.

[Peter Collingbourne]

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:168
@@ +167,3 @@
+LowerBitSets::BitSetInfo LowerBitSets::buildBitSet(
+    Module &M,
+    MDString *BitSet,
----------------
kcc wrote:
> do you need M here? 
No, I'll remove it.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:197
@@ +196,3 @@
+  }
+
+  if (Min > Max)
----------------
kcc wrote:
> I'd split this function into two at this point. 
> Then the second part of the function will become easily unit-testable. 
Makes sense, I'll do that.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:201
@@ +200,3 @@
+
+  // Normalize each offset against the minimum observed offset, and compute
+  // the bitwise OR of each of the offsets. The number of trailing zeros
----------------
kcc wrote:
> This part is the key to understanding what we are doing here. 
> It would be nice to have examples of input (array of offsets) and output (bitset). 
> But then, a unit tests would be a better example than comments.
> I do not agree with you here that we have enough test coverage and hence do not need more tests. 
> unit tests are not necessarily required only for test coverage -- they are also a good way to provide examples.  
Fair enough, I'll see if I can add a few unit tests.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:293
@@ +292,3 @@
+  // instruction with the previously computed bit offset; LLVM does not
+  // currently expose this instruction.
+  Value *BitSetGlobalOffset = ThenB.CreateLShr(
----------------
kcc wrote:
> the code gen seems to generate the BT instruction sometimes. 
> 
> It's may be better to just create an instruction sequence that will be lowered to BT.
> Example: 
> 
> void foo(unsigned char a, unsigned long b) {
>   if (a & (1 << b))
>     __builtin_trap();
> }
> 
> 
>    0:	0f a3 f7             	bt     %esi,%edi
>    3:	72 01                	jb     6 <_Z3foohm+0x6>
>    5:	c3                   	retq   
>    6:	0f 0b                	ud2    
> 
> 
> 
Yes, we can do that. I did try before to emit the register variant of BT but I guess I wasn't using the correct pattern. I'll play around with the IR to see if I can get it to match.

I was also thinking more of the memory variant of BT that basically does exactly what we want given a bit offset.

http://reviews.llvm.org/D7288

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