[CodeGen] Compute DT/LI lazily in SafeStackLegacyPass. NFC.

[ab]

Summary

We currently require SCEV, which requires DT/LI. Those are expensive to compute, but the pass only runs for functions that have the safestack attribute.
Compute DT/LI to build SCEV lazily, only when the pass is actually going to transform the function.

This updates a test I added in my local branch, that checks the entire O0 llc x86 pipeline. It's a little overzealous, but hopefully that makes changes to the O0 pipeline obvious; it's easy to require expensive passes, and this helps make informed decisions.

[ab]

Ping!

[davide]

Do you have #s on the compile time impact? I'd rather err on the side of safety (i.e. recomputing each time) unless the impact is substantial (I assume it is, but I'm just curious and I'd like to see the #s).

[ab]

Of course, sorry, here's some context: Michael and I noticed 4 unnecessary DT computations at O0. Altogether, they added up to a non-trivial compile-time win on the CTMark subset of the test-suite: 1.3-4.7%, 2.5% geomean.

This removes one of the 4 DT uses. Here are patches for the other two: D31641, D31642.

I think this is awkward, but it seems like a reasonably self-contained stopgap until we have cached lazy analyzes. (the least bad alternative I came up with was to add a TargetOption for safestack, and disable it from clang if we weren't passed -fsanitize=safestack. But that has a bunch of edge cases we might as well avoid, for llc, LTO, and clang on IR inputs).

To be clear, I don't think this is acceptable if it becomes pervasive: it seems like these backend passes are exceptions in that they're part of the O0 pipeline, only run at a fixed location in the pipeline, and are known to follow other pre-isel passes that don't preserve anything. More importantly, we have a cleaner path forward with the new pass manager.

[chandlerc]

FWIW, I'm pretty happy with this approach especially for something triggered by an attribute like this. It is a bit gross, but doesn't seem *that* gross. Still, should see if your explanation satisfies davide. =]

[davide]

LGTM

[pcc]

LGTM as well

[ab]

Thanks folks! No objections for D31303 then?

llvm/trunk/lib/CodeGen/SafeStack.cpp

public:
   static char ID; // Pass identification, replacement for typeid..
   SafeStackLegacyPass(const TargetMachine *TM) : FunctionPass(ID), TM(TM) {
     initializeSafeStackLegacyPassPass(*PassRegistry::getPassRegistry());
   }
 
   SafeStackLegacyPass() : SafeStackLegacyPass(nullptr) {}
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.addRequired<ScalarEvolutionWrapperPass>();
+    AU.addRequired<TargetLibraryInfoWrapperPass>();
+    AU.addRequired<AssumptionCacheTracker>();
   }
 
   bool runOnFunction(Function &F) override {
     DEBUG(dbgs() << "[SafeStack] Function: " << F.getName() << "\n");
 
     if (!F.hasFnAttribute(Attribute::SafeStack)) {
       DEBUG(dbgs() << "[SafeStack]     safestack is not requested"
                       " for this function\n");
       return false;
     }
 
     if (F.isDeclaration()) {
       DEBUG(dbgs() << "[SafeStack]     function definition"
                       " is not available\n");
       return false;
     }
 
     if (!TM)
       report_fatal_error("Target machine is required");
     auto *TL = TM->getSubtargetImpl(F)->getTargetLowering();
     if (!TL)
       report_fatal_error("TargetLowering instance is required");
 
     auto *DL = &F.getParent()->getDataLayout();
-    auto &SE = getAnalysis<ScalarEvolutionWrapperPass>().getSE();
+    auto &TLI = getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
+    auto &ACT = getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
+
+    // Compute DT and LI only for functions that have the attribute.
+    // This is only useful because the legacy pass manager doesn't let us
+    // compute analyzes lazily.
+    // In the backend pipeline, nothing preserves DT before SafeStack, so we
+    // would otherwise always compute it wastefully, even if there is no
+    // function with the safestack attribute.
+    DominatorTree DT(F);
+    LoopInfo LI(DT);
+
+    ScalarEvolution SE(F, TLI, ACT, DT, LI);
 
     return SafeStack(F, *TL, *DL, SE).run();
   }
 };
 
 } // anonymous namespace
 
 char SafeStackLegacyPass::ID = 0;
 INITIALIZE_TM_PASS_BEGIN(SafeStackLegacyPass, "safe-stack",
                          "Safe Stack instrumentation pass", false, false)
 INITIALIZE_TM_PASS_END(SafeStackLegacyPass, "safe-stack",
                        "Safe Stack instrumentation pass", false, false)
 
 FunctionPass *llvm::createSafeStackPass(const llvm::TargetMachine *TM) {
   return new SafeStackLegacyPass(TM);
 }

llvm/trunk/test/CodeGen/X86/O0-pipeline.ll

 ; CHECK-NEXT:       Lower Garbage Collection Instructions
 ; CHECK-NEXT:       Shadow Stack GC Lowering
 ; CHECK-NEXT:       Remove unreachable blocks from the CFG
 ; CHECK-NEXT:       Inserts calls to mcount-like functions
 ; CHECK-NEXT:     Rewrite Symbols
 ; CHECK-NEXT:     FunctionPass Manager
 ; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Exception handling preparation
-; CHECK-NEXT:       Dominator Tree Construction
-; CHECK-NEXT:       Natural Loop Information
-; CHECK-NEXT:       Scalar Evolution Analysis
 ; CHECK-NEXT:       Safe Stack instrumentation pass
 ; CHECK-NEXT:       Insert stack protectors
 ; CHECK-NEXT:       Module Verifier
 ; CHECK-NEXT:       Dominator Tree Construction
 ; CHECK-NEXT:       Basic Alias Analysis (stateless AA impl)
 ; CHECK-NEXT:       Function Alias Analysis Results
 ; CHECK-NEXT:       X86 DAG->DAG Instruction Selection
 ; CHECK-NEXT:       X86 PIC Global Base Reg Initialization