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

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:227
@@ -223,2 +226,3 @@
 
   // Mapping from bitset mdstrings to the call sites that test them.
+  DenseMap<Metadata *, std::vector<CallInst *>> BitSetTestCallSites;
----------------
Comment is out of date.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:571
@@ +570,3 @@
+    Constant *CombinedGlobalIdxs[] = {ConstantInt::get(Int32Ty, 0),
+                                      ConstantInt::get(Int32Ty, I * 2)};
+    Constant *CombinedGlobalElemPtr = ConstantExpr::getGetElementPtr(
----------------
I'm not sure I understand padding elements. Could you explain further?

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:579
@@ +578,3 @@
+          GlobalAlias::create(Globals[I]->getType(), Globals[I]->getLinkage(),
+                              "", CombinedGlobalElemPtr, M);
+      GAlias->takeName(Globals[I]);
----------------
Does this work with thread-local globals?
I'm also a bit worried about visibility / section / linkage type, but I may be misunderstanding the implications of what this patch adds.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:589
@@ +588,3 @@
+  if (Arch != Triple::x86 && Arch != Triple::x86_64)
+    report_fatal_error("Unsupported architecture for jump tables");
+
----------------
Could this be a virtual function instead? `report_fatal_error` by default, and only implemented for x86. I'm assuming the creation will also need to be specialized.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:651
@@ +650,3 @@
+void LowerBitSets::buildBitSetsFromFunctions(
+    ArrayRef<Metadata *> BitSets, ArrayRef<Function *> Globals) {
+  // This simple layout is based on the regular layout of jump tables.
----------------
Can Globals ever be empty? It seems a bit silly, but not entirely unlikely if someone just runs the pass.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:666
@@ +665,3 @@
+        FunctionType::get(Type::getVoidTy(M->getContext()), /*isVarArg=*/false),
+        Function::PrivateLinkage, "", M);
+    BasicBlock *BB = BasicBlock::Create(M->getContext(), "", NewFn);
----------------
Give it some dummy internal name starting with `__` so it's easier to figure things out. I'm assuming there's a test that'll check for that function name too.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:695
@@ +694,3 @@
+  // references to the original functions with references to the aliases.
+  for (unsigned I = 0; I != Globals.size()-1; ++I) {
+    Constant *CombinedGlobalElemPtr = ConstantExpr::getIntToPtr(
----------------
`clang-format`, here and a few other places.

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:889
@@ +888,3 @@
+    // Order bitsets by BitSetNM index for determinism.
+    std::sort(BitSets.begin(), BitSets.end(), [&](Metadata *M1, Metadata *M2) {
+      return BitSetIdIndices[M1] < BitSetIdIndices[M2];
----------------
IIUC duplicate indices can't occur?

================
Comment at: lib/Transforms/IPO/LowerBitSets.cpp:906
@@ -739,1 +905,3 @@
+                       reinterpret_cast<Function **>(OrderedGlobals.data() +
+                                                     OrderedGlobals.size())));
   }
----------------
Eek, the above lines make me cry. LLVM doesn't have a way to create `ArrayRef` taking into account its magical type hierarchy? It would be nice to avoid the `reinterpret_cast` here, since it relies heavily on the `GlobalObject` / `GlobalVariable` / `Function` class hierarchy layout...


http://reviews.llvm.org/D11856