[PATCH] D57463: Add a module pass for order file instrumentation

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:751
 
+  if (EnableOrderFileInstrumentation)
+    MPM.addPass(InstrOrderFilePass());
----------------
The pass should probably be moved after partial inlining and eliminateAvailableExternallyPass.  Partial inlining may create new functions, and it is also unnecessary to instrument availableExternally functions.


================
Comment at: lib/Transforms/Instrumentation/InstrOrderFile.cpp:80
+    // Create the global variables.
+    std::string SymbolName = "order_file_buffer";
+    OrderFileBuffer = new GlobalVariable(M, BufferTy, false, GlobalValue::LinkOnceODRLinkage,
----------------
Should define macro in InstProfData.inc file. 

The name of the symbol is also too generic. Need to prefix with __llvm_...


================
Comment at: lib/Transforms/Instrumentation/InstrOrderFile.cpp:87
+
+    std::string IndexName = "order_file_buffer_idx";
+    BufferIdx = new GlobalVariable(M, IdxTy, false, GlobalValue::LinkOnceODRLinkage,
----------------
Same here.


================
Comment at: lib/Transforms/Instrumentation/InstrOrderFile.cpp:147
+    // We need to wrap around the index to fit it inside the buffer.
+    Value *WrappedIdx = updateB.CreateAnd(
+        IdxVal, ConstantInt::get(Int32Ty, INSTR_ORDER_FILE_BUFFER_MASK));
----------------
Why not having a runtime interface to flush the buffer here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57463/new/

https://reviews.llvm.org/D57463