[PATCH] D21053: IR: New representation for CFI and virtual call optimization pass metadata.

[Evgeniy Stepanov via llvm-commits]

eugenis added inline comments.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:52
@@ -51,3 +51,3 @@
 
-  ConstantInt *extractBitSetTypeId(MDNode *MD);
+  ConstantInt *extractBitSetTypeId(GlobalObject &GO, MDNode *MD);
   void buildCFICheck();
----------------
rename to extractTypeMetadataTypeId?

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:87
@@ -86,10 +86,3 @@
 
-  // Sanity check.
-  auto FM = dyn_cast_or_null<ValueAsMetadata>(MD->getOperand(1));
-  // Can be null if a function was removed by an optimization.
-  if (FM) {
-    auto F = dyn_cast<Function>(FM->getValue());
-    // But can never be a function declaration.
-    assert(!F || !F->isDeclaration());
-    (void)F; // Suppress unused variable warning in the no-asserts build.
-  }
+  // Sanity check. GO must not be a function declaration.
+  auto F = dyn_cast<Function>(&GO);
----------------
Maybe move this check to the caller? This function does not really use the GlobalObject argument.

================
Comment at: lib/Transforms/IPO/CrossDSOCFI.cpp:98
@@ -103,1 +97,3 @@
+  // but before the jump slots created in LowerTypeTests.
   llvm::DenseSet<uint64_t> BitSetIds;
+  SmallVector<MDNode *, 2> Types;
----------------
There are still some references to "BitSet" in this file. Is it intentional?
I understand the devirt thing is using BitSet to refer to the bits stored near the virtual table, which is a completely separate concept from the type test metadata.

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:832
@@ +831,3 @@
+  for (GlobalObject *GO : Globals) {
+    // Op = { offset, bitset name }
+    Types.clear();
----------------
This comment is now confusing, there is no "Op".


================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:906
@@ -937,1 +905,3 @@
+  // Verify the type metadata and build a mapping from bitset identifiers to
+  // their last observed index in the list of globals. This will used later to
   // deterministically order the list of bitset identifiers.
----------------
"will be used later"

================
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:960
@@ -988,1 +959,3 @@
+    for (GlobalVariable &GV : M->globals())
+      AddGlobalToEqClass(GV);
   }
----------------
Would you like to factor out this code that applies a function to all functions and globals in a module?


http://reviews.llvm.org/D21053