[PATCH] D20892: [esan|cfrag] Instrument GEP instr for struct field access.

[Qin Zhao via llvm-commits]

zhaoqin marked an inline comment as done.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:329
@@ +328,3 @@
+    // Remember the counter variable for each struct type.
+    StructTyMap.insert(std::pair<Type *, GlobalVariable *>(StructTy, Counters));
+
----------------
aizatsky wrote:
> Will this grow and grow in huge lto programs? Is this a concern?
This StructTyMap should be fairly small esp if separate compilation.

If there is any concern, that would be that we have one struct counter array for each struct type and we rely on the linker to merge them based on their name, xref createStructCounterName.
Another concern is the runtime hash map for holding each struct type counter.
I would expect the total number of different struct/class types won't be an insane number.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:613
@@ +612,3 @@
+  if (GepInst == nullptr || !isa<StructType>(GepInst->getSourceElementType()) ||
+      StructTyMap.count(GepInst->getSourceElementType()) == 0 ||
+      !GepInst->hasAllConstantIndices() ||
----------------
aizatsky wrote:
> It is not clear to me why this map would contain this source element type at all. Wouldn't this work only for structs defined in the same module?
I assume that StructTyMap would have that type (i.e, struct type is in M.getIdentifiedStructTypes()) if the GEP instr knows how to calculate the field offset within the struct.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:616
@@ +615,3 @@
+      // Only handle simple struct field GEP.
+      GepInst->getNumIndices() != 2)
+    return false;
----------------
aizatsky wrote:
> Do you need counters for how many accesses were skipped?
add NumIgnoredGEPs and NumInstrumentedGEPs.


http://reviews.llvm.org/D20892