LibcallsShrinkWrap doesn't preserve the CFG

[davide]

Summary

This is the dominator before/after

Computed:

Inorder Dominator Tree:

[1] %0 {0,1}

Actual:

Inorder Dominator Tree:

[1] %0 {0,5}
  [2] %cdce.call {1,2}
  [2] %cdce.end {3,4}

[davide]

Updated testcase.

[xur]

Thanks for the fix. Look good to me.
It may be better to rename the test case using bug_id.

[efriedma]

Can you use -verify-dom-info to write a testcase, as opposed to a run line with three passes? (If that doesn't actually work, please ignore this.)

LibcallsShrinkWrap should preserve GlobalsAAWrapperPass, at least. domtree would probably be easy to preserve, but that can be done as a followup.

Please make sure to upload patches with full context.

[davide]

Update after Eli's comments.

[davide]

In D26381#588903, @efriedma wrote:

Can you use -verify-dom-info to write a testcase, as opposed to a run line with three passes? (If that doesn't actually work, please ignore this.)

I tried before, but that doesn't actually work.

LibcallsShrinkWrap should preserve GlobalsAAWrapperPass, at least. domtree would probably be easy to preserve, but that can be done as a followup.

Done.

Please make sure to upload patches with full context.

Sorry, for some reason I forgot to add -U to my git diff invocation..

[davide]

Renamed test file.

[efriedma]

Looked into the test a little; I guess to trigger -verify-dom-info, you need to compute domtree before -libcalls-shrinkwrap, and have a pass which requires domtree after it. So "opt -domtree -libcalls-shrinkwrap -instcombine -verify-dom-info" triggers verification, for example. I think that's a little bit better than dragging in gvn.

[davide]

Updated the pipeline. Also added a comment to explain what's going on there.

[efriedma]

LGTM.

I feel like we should probably make a new directory for libcalls-shrinkwrap tests at some point, but you don't have to do that here.

lib/Transforms/Utils/LibCallsShrinkWrap.cpp

 //  These partially dead calls are usually results of C++ abstraction penalty
 //  exposed by inlining.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/Utils/LibCallsShrinkWrap.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/Analysis/GlobalsModRef.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstVisitor.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
bool LibCallsShrinkWrap::perform(CallInst *CI) {
 
   if (LibCallsShrinkWrapDoRangeError && performCallRangeErrorOnly(CI, Func))
     return true;
 
   return performCallErrors(CI, Func);
 }
 
 void LibCallsShrinkWrapLegacyPass::getAnalysisUsage(AnalysisUsage &AU) const {
-  AU.setPreservesCFG();
+  AU.addPreserved<GlobalsAAWrapperPass>();
   AU.addRequired<TargetLibraryInfoWrapperPass>();
 }
 
 bool runImpl(Function &F, const TargetLibraryInfo &TLI) {
   if (F.hasFnAttribute(Attribute::OptimizeForSize))
     return false;
   LibCallsShrinkWrap CCDCE(TLI);
   CCDCE.visit(F);

 }
 
 PreservedAnalyses LibCallsShrinkWrapPass::run(Function &F,
                                               FunctionAnalysisManager &FAM) {
   auto &TLI = FAM.getResult<TargetLibraryAnalysis>(F);
   bool Changed = runImpl(F, TLI);
   if (!Changed)
     return PreservedAnalyses::all();
-  return PreservedAnalyses::none();
+  auto PA = PreservedAnalyses();
+  PA.preserve<GlobalsAA>();
+  return PA;
 }
 }

test/Transforms/InstCombine/pr30929.ll

+; We need this pipeline because to trigger dominator info verification
+; we have to compute the dominator before libcalls-shrinkwrap and
+; have a pass which requires the dominator tree after.
+; RUN: opt -domtree -libcalls-shrinkwrap -instcombine -verify-dom-info %s
+
+define void @main() {
+  %_tmp31 = call float @acosf(float 2.000000e+00)
+  ret void
+}
+
+declare float @acosf(float)