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

[Ivan Krasin via llvm-commits]

krasin added a subscriber: krasin.
krasin added a comment.

A few shallow comments...


================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:609
@@ +608,3 @@
+
+  ConstantInt *Jmp = ConstantInt::get(Int8Ty, 0xe9);
+
----------------
What is 0xe9? 

================
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());
----------------
What happens, if the last function isDiscardableIfUnused? Could it be that a consequent pass would drop the whole jump table?


================
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);
----------------
May be a lame question: what if __function_bitset_dummy is taken?

================
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.
+
----------------
a minor typo: s/aare/are/

================
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
----------------
Other newly added tests have CHECK statements inside foo / bar / baz functions, and this test does not. Is it intentional?


http://reviews.llvm.org/D11856