[pipelines] Require GlobalsAA after sanitizers

[vitalybuka]

Summary

Restore GlobalsAA if sanitizers inserted at early optimize callback.
The analysis can be useful for the following FunctionPassManager.

[aeubanks]

I think the question is whether or not people will use OptimizerEarlyEPCallbacks to add optimization passes (e.g. some sort of loop vectorization) which may want an up-to-date GlobalsAA, or if the passes will end up helping make GlobalsAA more precise via more simplification.
Is this in preparation for actually using OptimizerEarlyEPCallbacks? Note that this won't help with having GlobalsAA updated if you add sanitizer passes here that invalidate GlobalsAA since RecomputeGlobalsAAPass only has an effect if GlobalsAA hasn't been explicitly invalidated (RecomputeGlobalsAAPass::run).

[vitalybuka]

In D133537#3781499, @aeubanks wrote:

I think the question is whether or not people will use OptimizerEarlyEPCallbacks to add optimization passes (e.g. some sort of loop vectorization) which may want an up-to-date GlobalsAA,

Yes, we need to choose either:

1. Callback which needs GlobalsAA will also insert RecomputeGlobalsAAPass (but how many times if we have multiple callbacks?)
2. Callback which corrupt GlobalAA will insert RecomputeGlobalsAAPass. (but how many times if we have multiple callbacks?)
3. Run RecomputeGlobalsAAPass before and after callbacks, but then it's unnecessary if we have no callbacks relevant to analysis.

This patch is to go no. 1.

Is this in preparation for actually using OptimizerEarlyEPCallbacks?

Yes.

In D133537#3781499, @aeubanks wrote:

Note that this won't help with having GlobalsAA updated if you add sanitizer passes here that invalidate GlobalsAA since RecomputeGlobalsAAPass only has an effect if GlobalsAA hasn't been explicitly invalidated (RecomputeGlobalsAAPass::run).

I assumed that D133394 invalidates (it's even in description). Then RecomputeGlobalsAAPass will recalculated it, but if RecomputeGlobalsAAPass is missing we will proceed without GlobalsAA.
But now I see that abandon<> removes it forever.

What would be the right solution for that?
I see the following:

1. Just for clang, I revert D133394 and just make sanitizer callbacks to insert RecomputeGlobalsAAPass after sanitizers.
2. Extract RecomputeGlobalsAAPass::run into utility function and call it from sanitizers instead of abandon<>

[aeubanks]

RecomputeGlobalsAAPass is essentially equivalent to "if GlobalsAA exists in the cache, abandon it and recompute", so it's similar to

if (GlobalsAA in analysis cache) {
 MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
 MPM.addPass(InvalidateAnalysisPass<GlobalsAA>());
}

since RecomputeGlobalsAAPass doesn't work if the analysis doesn't exist in the cache, if we've added a sanitizer that may abandon GlobalsAA, we can just add a
MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>()); in BackendUtil.cpp/SanitizersCallback which will fully compute GlobalsAA from scratch

that would be better than this patch because OptimizerEarlyEPCallbacks still has an up-to-date GlobalsAA, but if we add sanitizers we'll still have an up-to-date GlobalsAA right after

[vitalybuka]

update

[vitalybuka]

In D133537#3786958, @aeubanks wrote:

RecomputeGlobalsAAPass is essentially equivalent to "if GlobalsAA exists in the cache, abandon it and recompute", so it's similar to

if (GlobalsAA in analysis cache) {
 MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
 MPM.addPass(InvalidateAnalysisPass<GlobalsAA>());
}

since RecomputeGlobalsAAPass doesn't work if the analysis doesn't exist in the cache, if we've added a sanitizer that may abandon GlobalsAA, we can just add a
MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>()); in BackendUtil.cpp/SanitizersCallback which will fully compute GlobalsAA from scratch

that would be better than this patch because OptimizerEarlyEPCallbacks still has an up-to-date GlobalsAA, but if we add sanitizers we'll still have an up-to-date GlobalsAA right after

Wouldn't be better to do so on PassBuilderPipelines like this, so we can cover any other future abandoning hooks?

[aeubanks]

In D133537#3787925, @vitalybuka wrote:

Wouldn't be better to do so on PassBuilderPipelines like this, so we can cover any other future abandoning hooks?

We don't do that for all callbacks, I don't see why OptimizerEarlyEPCallbacks should be special. It's not typical to add passes that add globals. That's why it feels like it belongs wherever we add sanitizers.

[vitalybuka]

In D133537#3788056, @aeubanks wrote:

In D133537#3787925, @vitalybuka wrote:

Wouldn't be better to do so on PassBuilderPipelines like this, so we can cover any other future abandoning hooks?

We don't do that for all callbacks, I don't see why OptimizerEarlyEPCallbacks should be special. It's not typical to add passes that add globals. That's why it feels like it belongs wherever we add sanitizers.

It's fine to me either way. However we don't do RecomputeGlobalsAAPass before other callback either.

[aeubanks]

In D133537#3788251, @vitalybuka wrote:

In D133537#3788056, @aeubanks wrote:

In D133537#3787925, @vitalybuka wrote:

Wouldn't be better to do so on PassBuilderPipelines like this, so we can cover any other future abandoning hooks?

We don't do that for all callbacks, I don't see why OptimizerEarlyEPCallbacks should be special. It's not typical to add passes that add globals. That's why it feels like it belongs wherever we add sanitizers.

It's fine to me either way. However we don't do RecomputeGlobalsAAPass before other callback either.

The existing RecomputeGlobalsAAPass is specifically because we want the module optimization pipeline to have as refined AA as possible.
I'd still prefer adding this wherever we add sanitizers.

[vitalybuka]

update

[vitalybuka]

remove typo

[aeubanks]

lg with comments addressed

title needs update

clang/lib/CodeGen/BackendUtil.cpp

443

unrelated?

[vitalybuka]

fix capture

[vitalybuka]

clang/lib/CodeGen/BackendUtil.cpp

443

yes, already fixed.

clang/lib/CodeGen/BackendUtil.cpp

static bool initTargetOptions(DiagnosticsEngine &Diags,
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.IgnoreXCOFFVisibility = LangOpts.IgnoreXCOFFVisibility;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
   Options.UniqueBasicBlockSectionNames =
       CodeGenOpts.UniqueBasicBlockSectionNames;
   Options.TLSSize = CodeGenOpts.TLSSize;
   Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
   Options.ExplicitEmulatedTLS = true;
   Options.DebuggerTuning = CodeGenOpts.getDebuggerTuning();
   Options.EmitStackSizeSection = CodeGenOpts.StackSizeSection;
   Options.StackUsageOutput = CodeGenOpts.StackUsageOutput;
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
   Options.ForceDwarfFrameSection = CodeGenOpts.ForceDwarfFrameSection;
   Options.EmitCallSiteInfo = CodeGenOpts.EmitCallSiteInfo;
   Options.EnableAIXExtendedAltivecABI = CodeGenOpts.EnableAIXExtendedAltivecABI;
   Options.XRayOmitFunctionIndex = CodeGenOpts.XRayOmitFunctionIndex;
auto MSanPass = [&](SanitizerMask Mask, bool CompileKernel) {
         MemorySanitizerOptions options(TrackOrigins, Recover, CompileKernel,
                                        CodeGenOpts.SanitizeMemoryParamRetval);
         MPM.addPass(MemorySanitizerPass(options));
         if (Level != OptimizationLevel::O0) {
           // MemorySanitizer inserts complex instrumentation that mostly follows
           // the logic of the original code, but operates on "shadow" values. It
           // can benefit from re-running some general purpose optimization
           // passes.
-          MPM.addPass(RecomputeGlobalsAAPass());
+          MPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
           FunctionPassManager FPM;
           FPM.addPass(EarlyCSEPass(true /* Enable mem-ssa. */));
           FPM.addPass(InstCombinePass());
           FPM.addPass(JumpThreadingPass());
           if (RunNewGVN)
             FPM.addPass(NewGVNPass());
           else
             FPM.addPass(GVNPass());
auto SanitizersCallback = [&](ModulePassManager &MPM,
     };
     HWASanPass(SanitizerKind::HWAddress, false);
     HWASanPass(SanitizerKind::KernelHWAddress, true);
 
     if (LangOpts.Sanitize.has(SanitizerKind::DataFlow)) {
       MPM.addPass(DataFlowSanitizerPass(LangOpts.NoSanitizeFiles));
     }
   };
-  if (ClSanitizeOnOptimizerEarlyEP)
-    PB.registerOptimizerEarlyEPCallback(SanitizersCallback);
-  else
+  if (ClSanitizeOnOptimizerEarlyEP) {
+    PB.registerOptimizerEarlyEPCallback(
+        [SanitizersCallback](ModulePassManager &MPM, OptimizationLevel Level) {
+          ModulePassManager NewMPM;
+          SanitizersCallback(NewMPM, Level);
+          if (!NewMPM.isEmpty()) {
+            // Sanitizers can abandon<GlobalsAA>.
+            NewMPM.addPass(RequireAnalysisPass<GlobalsAA, Module>());
+            MPM.addPass(std::move(NewMPM));
+          }
+        });
+  } else {
+    // LastEP does not need GlobalsAA.
     PB.registerOptimizerLastEPCallback(SanitizersCallback);
   }
+}
 
 void EmitAssemblyHelper::RunOptimizationPipeline(
     BackendAction Action, std::unique_ptr<raw_pwrite_stream> &OS,
     std::unique_ptr<llvm::ToolOutputFile> &ThinLinkOS) {
   Optional<PGOOptions> PGOOpt;
 
   if (CodeGenOpts.hasProfileIRInstr())
     // -fprofile-generate.