[PATCH] D11856: LowerBitSets: Extend pass to support functions as bitset members.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:609
@@ +608,3 @@
+
+  ConstantInt *Jmp = ConstantInt::get(Int8Ty, 0xe9);
+
----------------
krasin wrote:
> What is 0xe9? 
This would be the opcode for the PC-relative jmp instruction. I thought this was clear enough from the variable name and how it is used, but maybe not?

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:725
@@ +724,3 @@
+  // To create a jump table for these functions, we pick one function
+  // arbitrarily (say, f) and instruct the LLVM code generator to output a jump
+  // table immediately before f's instructions. This is done by representing the
----------------
pcc wrote:
> kcc wrote:
> > Why do we do this? 
> > 
> > Yes, this will reduce the size of the jump table by 8 bytes, 
> > but is this optimization worth the extra complexity? 
> > We must try to be as simple as possible 
> > 
> The motivation wasn't so much optimization but the fact that the jump table is guaranteed to appear in an executable section if we do this. However, if it would work just to set the section name it may be best to do that instead (and fix it if not). Let me take a look.
Okay, done. The code is much simpler now.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:781
@@ +780,3 @@
+  // function and use that as the last function.
+  if (Globals.back()->isDeclarationForLinker()) {
+    NewGlobals.insert(NewGlobals.end(), Globals.begin(), Globals.end());
----------------
krasin wrote:
> What happens, if the last function isDiscardableIfUnused? Could it be that a consequent pass would drop the whole jump table?
> 
This comment is obsoleted by the move to a global variable for the jump table. (But any uses of the jump table would also count as uses of the function, so this shouldn't matter.)

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:785
@@ +784,3 @@
+        FunctionType::get(Type::getVoidTy(M->getContext()), /*isVarArg=*/false),
+        Function::PrivateLinkage, "__function_bitset_dummy", M);
+    BasicBlock *BB = BasicBlock::Create(M->getContext(), "", NewFn);
----------------
krasin wrote:
> May be a lame question: what if __function_bitset_dummy is taken?
Also obsolete

================
Comment at: test/Transforms/LowerBitSets/function-ext.ll:4
@@ +3,3 @@
+; Tests that wr correctly hande external references, including the case where
+; all functions in a bitset aare external references.
+
----------------
krasin wrote:
> a minor typo: s/aare/are/
Fixed and a few other typos here

================
Comment at: test/Transforms/LowerBitSets/nonstring.ll:21
@@ +20,3 @@
+define i1 @foo(i8* %p) {
+  %x = call i1 @llvm.bitset.test(i8* %p, metadata !2)
+  ret i1 %x
----------------
krasin wrote:
> Other newly added tests have CHECK statements inside foo / bar / baz functions, and this test does not. Is it intentional?
I suppose we could do that here as well; done.


http://reviews.llvm.org/D11856