[PATCH] D56470: [NewPM] Second attempt at porting ASan

[Philip Pfaffe via Phabricator via llvm-commits]

philip.pfaffe added a comment.

This is starting to look great! Minor nits inline.

Don't worry about copying the Result! Returning usually doesn't copy, but that's not really the point. Analyses are only ran if their result is missing or invalid, and the AnalysisManager caches analysis results! That's the absolute beauty of it!



================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:529
 
-/// AddressSanitizer: instrument the code in module to find memory bugs.
-struct AddressSanitizer : public FunctionPass {
-  // Pass identification, replacement for typeid
+bool ShouldCompileKernel(bool CompileKernel) {
+  return ClEnableKasan.getNumOccurrences() > 0 ? ClEnableKasan : CompileKernel;
----------------
No need to pull this out!

As a followup, I want to add options handling just like we did for loop unrolling. Then this will get unified in a much nicer way!


================
Comment at: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp:1141
+          MAM.getCachedResult<ASanGlobalsMetadataAnalysis>(*F.getParent())) {
+    GlobalsMetadata &GlobalsMD = *R;
+    const TargetLibraryInfo *TLI = &AM.getResult<TargetLibraryAnalysis>(F);
----------------
This can be made a bit simpler. No need for an extra GlobalsMD variable, and you don't need to call F.getParent() twice.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56470