Explicitly require DominatorTreeAnalysis pass for instsimplify pass.

[danielcdh]

Summary

DominatorTreeAnalysis is always required by instsimplify.

[danielcdh]

commit without review.

[danielcdh]

saw a bunch of buildbot failure pointing to this one, thus revert the change, investigating...

[danielcdh]

Looks like the buildbot failure are due to the other patch. But I'll hold off until I get an approval before I commit the patch again.

Change #123918

Changed by alexshap
Changed at Thu 01 Sep 2016 17:00:30
Repository http://llvm.org/svn/llvm-project
Project clang-tools-extra
Branch trunk
Revision 280431
Comments

Add clang-reorder-fields to clang-tools-extra

This diff adds v0 of clang-reorder-fields tool to clang/tools/extra.
The main idea behind this tool is to simplify and make less error-prone refactoring of large codebases when
someone needs to change the order fields of a struct/class (for example to remove excess padding).

Differential revision: https://reviews.llvm.org/D23279
Changed files

CMakeLists.txt
clang-reorder-fields
clang-reorder-fields/CMakeLists.txt
clang-reorder-fields/ReorderFieldsAction.cpp
clang-reorder-fields/ReorderFieldsAction.h
clang-reorder-fields/tool
clang-reorder-fields/tool/CMakeLists.txt
clang-reorder-fields/tool/ClangReorderFields.cpp
test/CMakeLists.txt
test/clang-reorder-fields
test/clang-reorder-fields/AggregatePartialInitialization.cpp
test/clang-reorder-fields/CStructAmbiguousName.cpp
test/clang-reorder-fields/CStructFieldsOrder.cpp
test/clang-reorder-fields/ClassDifferentFieldsAccesses.cpp
test/clang-reorder-fields/ClassMixedInitialization.cpp
test/clang-reorder-fields/ClassSimpleCtor.cpp

[danielcdh]

ping...

[davidxl]

What breaks without the change?

[danielcdh]

In D24173#535251, @davidxl wrote:

What breaks without the change?

When I enable LoopSink pass, I need to add InstSimplifierPass pass after it to do some cleanup. However, if I directly invoke InstSimplifierPass pass, it will fail due to lack of DT.

From the InstSimplifierPass implementation, it uses DT without checking if it is nullptr. So I think we should explicitly require DT for this pass.

A typical use of DT from InstSimplifierPass is:

llvm::SimplifyInstruction->
llvm::SimplifyICmpInst->
SimplifyICmpInst ->
computePointerICmp->
llvm::isKnownNonNullAt (ValueTracking.cpp) ->
isKnownNonNullFromDominatingCondition (ValueTracking.cpp) ->
llvm::DominatorTree::dominates (Dominators.cpp)

[davidxl]

lgtm

[chandlerc]

The change seems fine, but you should add a test that demonstrates the failure. Any IR input that triggers the code path you suggest run with just the inst simplify pass from opt should suffice?

[danielcdh]

Test added in r280963.

lib/Transforms/Utils/SimplifyInstructions.cpp

namespace {
   struct InstSimplifier : public FunctionPass {
     static char ID; // Pass identification, replacement for typeid
     InstSimplifier() : FunctionPass(ID) {
       initializeInstSimplifierPass(*PassRegistry::getPassRegistry());
     }
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
       AU.setPreservesCFG();
+      AU.addRequired<DominatorTreeWrapperPass>();
       AU.addRequired<AssumptionCacheTracker>();
       AU.addRequired<TargetLibraryInfoWrapperPass>();
     }
 
     /// runOnFunction - Remove instructions that simplify.
     bool runOnFunction(Function &F) override {
       if (skipFunction(F))
         return false;
 
-      const DominatorTreeWrapperPass *DTWP =
-          getAnalysisIfAvailable<DominatorTreeWrapperPass>();
-      const DominatorTree *DT = DTWP ? &DTWP->getDomTree() : nullptr;
+      const DominatorTree *DT =
+          &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
       const TargetLibraryInfo *TLI =
           &getAnalysis<TargetLibraryInfoWrapperPass>().getTLI();
       AssumptionCache *AC =
           &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
       return runImpl(F, DT, TLI, AC);
     }
   };
 }
 
 char InstSimplifier::ID = 0;
 INITIALIZE_PASS_BEGIN(InstSimplifier, "instsimplify",
                       "Remove redundant instructions", false, false)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
+INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
 INITIALIZE_PASS_END(InstSimplifier, "instsimplify",
                     "Remove redundant instructions", false, false)
 char &llvm::InstructionSimplifierID = InstSimplifier::ID;
 
 // Public interface to the simplify instructions pass.
 FunctionPass *llvm::createInstructionSimplifierPass() {
   return new InstSimplifier();
 }
 
 PreservedAnalyses InstSimplifierPass::run(Function &F,
                                       FunctionAnalysisManager &AM) {
-  auto *DT = AM.getCachedResult<DominatorTreeAnalysis>(F);
+  auto &DT = AM.getResult<DominatorTreeAnalysis>(F);
   auto &TLI = AM.getResult<TargetLibraryAnalysis>(F);
   auto &AC = AM.getResult<AssumptionAnalysis>(F);
-  bool Changed = runImpl(F, DT, &TLI, &AC);
+  bool Changed = runImpl(F, &DT, &TLI, &AC);
   if (!Changed)
     return PreservedAnalyses::all();
   // FIXME: This should also 'preserve the CFG'.
   return PreservedAnalyses::none();
 }