[SimplifyLibCalls][IRBuilder] Accept any IRBuilder in SimplifyLibCalls

[nikic]

Summary

This changes the SimplifyLibCalls utility to accept an IRBuilderBase, which allows us to pass through the IRBuilder used by InstCombine. This will ensure that new instructions get added to the worklist. The annotated test-case drops from 4 to 2 InstCombine iterations thanks to this.

To achieve this, I'm adding an IRBuilderBase::OperandBundlesGuard, which is basically the same as the existing InsertPointGuard and FastMathFlagsGuard, but for operand bundles. Also add a setDefaultOperandBundles() method so these can be set outside the constructor.

[nhaehnle]

That's a nice improvement. I do have one detail question.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

3109–3111

The OperandBundlesGuard covers this point, where bundles where previously not applied as far as I can tell. Is that intended?

[nikic]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

3109–3111

It looks like the separate builder was introduced in https://github.com/llvm/llvm-project/commit/b491a2d641add36f1a2f7674d6b5df038c2d638e to fix the insertion position, and the later introduction of operand bundle handling in https://github.com/llvm/llvm-project/commit/b70e23c3909ca9a848a7ba963ee6580f48c57c5f forgot to adjust this separate builder. I don't think that operand bundles were intentionally dropped here. (Though most likely nobody is using operand bundles on fortified library calls in the first place.)

[nhaehnle]

LGTM

[uabelho]

Hi,

Is this supposed to affect the output from instcombine?

When I merged this to our downstream clone I noticed that it does.
If I run instcombine on the following input:

declare i32 @strlen(i8*)

define void @test_it() {
bb1:
  %_tmp12 = call i32 @strlen(i8* undef)
  ret void
}

with

opt -instcombine -S -o - strlen.ll

I got

define void @test_it() {
bb1:
  ret void
}

before, but with this patch I now get

define void @test_it() {
bb1:
  store i8 undef, i8* null, align 536870912
  ret void
}

instead.

[nikic]

@uabelho As this patch changes worklist order, that can also have an impact on the optimization result. For your example, the strlen is folded to zext of load. If the zext is DCEd first, the load is also DCEd (before this patch) and you end up with an empty function. If the load is processed first, it will fold to unreachable instead (after this patch). D75008 will change the order again to perform DCE first, in which case the previous behavior will be restored. Both outcomes are generally fine though.

[uabelho]

@nikic Alright, thanks!

llvm/include/llvm/IR/IRBuilder.h

void setConstrainedFPFunctionAttr() {
     }
   }
 
   void setConstrainedFPCallAttr(CallInst *I) {
     if (!I->hasFnAttr(Attribute::StrictFP))
       I->addAttribute(AttributeList::FunctionIndex, Attribute::StrictFP);
   }
 
+  void setDefaultOperandBundles(ArrayRef<OperandBundleDef> OpBundles) {
+    DefaultOperandBundles = OpBundles;
+  }
+
   //===--------------------------------------------------------------------===//
   // RAII helpers.
   //===--------------------------------------------------------------------===//
 
   // RAII object that stores the current insertion point and restores it
   // when the object is destroyed. This includes the debug location.
   class InsertPointGuard {
     IRBuilderBase &Builder;
public:
     FastMathFlagGuard &operator=(const FastMathFlagGuard &) = delete;
 
     ~FastMathFlagGuard() {
       Builder.FMF = FMF;
       Builder.DefaultFPMathTag = FPMathTag;
     }
   };
 
+  // RAII object that stores the current default operand bundles and restores
+  // them when the object is destroyed.
+  class OperandBundlesGuard {
+    IRBuilderBase &Builder;
+    ArrayRef<OperandBundleDef> DefaultOperandBundles;
+
+  public:
+    OperandBundlesGuard(IRBuilderBase &B)
+        : Builder(B), DefaultOperandBundles(B.DefaultOperandBundles) {}
+
+    OperandBundlesGuard(const OperandBundlesGuard &) = delete;
+    OperandBundlesGuard &operator=(const OperandBundlesGuard &) = delete;
+
+    ~OperandBundlesGuard() {
+      Builder.DefaultOperandBundles = DefaultOperandBundles;
+    }
+  };
+
+
   //===--------------------------------------------------------------------===//
   // Miscellaneous creation methods.
   //===--------------------------------------------------------------------===//
 
   /// Make a new global variable with initializer type i8*
   ///
   /// Make a new global variable with an initializer that has array of i8 type
   /// filled in with the null terminated string value specified.  The new global

llvm/include/llvm/Transforms/Utils/SimplifyLibCalls.h

 public:
   FortifiedLibCallSimplifier(const TargetLibraryInfo *TLI,
                              bool OnlyLowerUnknownSize = false);
 
   /// Take the given call instruction and return a more
   /// optimal value to replace the instruction with or 0 if a more
   /// optimal form can't be found.
   /// The call must not be an indirect call.
-  Value *optimizeCall(CallInst *CI);
+  Value *optimizeCall(CallInst *CI, IRBuilderBase &B);
 
 private:
   Value *optimizeMemCpyChk(CallInst *CI, IRBuilderBase &B);
   Value *optimizeMemMoveChk(CallInst *CI, IRBuilderBase &B);
   Value *optimizeMemSetChk(CallInst *CI, IRBuilderBase &B);
 
   /// Str/Stp cpy are similar enough to be handled in the same functions.
   Value *optimizeStrpCpyChk(CallInst *CI, IRBuilderBase &B, LibFunc Func);
public:
 
   /// optimizeCall - Take the given call instruction and return a more
   /// optimal value to replace the instruction with or 0 if a more
   /// optimal form can't be found.  Note that the returned value may
   /// be equal to the instruction being optimized.  In this case all
   /// other instructions that use the given instruction were modified
   /// and the given instruction is dead.
   /// The call must not be an indirect call.
-  Value *optimizeCall(CallInst *CI);
+  Value *optimizeCall(CallInst *CI, IRBuilderBase &B);
 
 private:
   // String and Memory Library Call Optimizations
   Value *optimizeStrCat(CallInst *CI, IRBuilderBase &B);
   Value *optimizeStrNCat(CallInst *CI, IRBuilderBase &B);
   Value *optimizeStrChr(CallInst *CI, IRBuilderBase &B);
   Value *optimizeStrRChr(CallInst *CI, IRBuilderBase &B);
   Value *optimizeStrCmp(CallInst *CI, IRBuilderBase &B);

llvm/lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::optimizeCallInst(CallInst *CI, bool &ModifiedDT) {
   // From here on out we're working with named functions.
   if (!CI->getCalledFunction()) return false;
 
   // Lower all default uses of _chk calls.  This is very similar
   // to what InstCombineCalls does, but here we are only lowering calls
   // to fortified library functions (e.g. __memcpy_chk) that have the default
   // "don't know" as the objectsize.  Anything else should be left alone.
   FortifiedLibCallSimplifier Simplifier(TLInfo, true);
-  if (Value *V = Simplifier.optimizeCall(CI)) {
+  IRBuilder<> Builder(CI);
+  if (Value *V = Simplifier.optimizeCall(CI, Builder)) {
     CI->replaceAllUsesWith(V);
     CI->eraseFromParent();
     return true;
   }
 
   return false;
 }
 

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

Instruction *InstCombiner::tryOptimizeCall(CallInst *CI) {
   auto InstCombineRAUW = [this](Instruction *From, Value *With) {
     replaceInstUsesWith(*From, With);
   };
   auto InstCombineErase = [this](Instruction *I) {
     eraseInstFromFunction(*I);
   };
   LibCallSimplifier Simplifier(DL, &TLI, ORE, BFI, PSI, InstCombineRAUW,
                                InstCombineErase);
-  if (Value *With = Simplifier.optimizeCall(CI)) {
+  if (Value *With = Simplifier.optimizeCall(CI, Builder)) {
     ++NumSimplified;
     return CI->use_empty() ? CI : replaceInstUsesWith(*CI, With);
   }
 
   return nullptr;
 }
 
 static IntrinsicInst *findInitTrampolineFromAlloca(Value *TrampMem) {

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

Value *LibCallSimplifier::optimizeFloatingPointLibCall(CallInst *CI,
   case LibFunc_cabsf:
   case LibFunc_cabsl:
     return optimizeCAbs(CI, Builder);
   default:
     return nullptr;
   }
 }
 
-Value *LibCallSimplifier::optimizeCall(CallInst *CI) {
+Value *LibCallSimplifier::optimizeCall(CallInst *CI, IRBuilderBase &Builder) {
   // TODO: Split out the code below that operates on FP calls so that
   //       we can all non-FP calls with the StrictFP attribute to be
   //       optimized.
   if (CI->isNoBuiltin())
     return nullptr;
 
   LibFunc Func;
   Function *Callee = CI->getCalledFunction();
+  bool isCallingConvC = isCallingConvCCompatible(CI);
 
   SmallVector<OperandBundleDef, 2> OpBundles;
   CI->getOperandBundlesAsDefs(OpBundles);
-  IRBuilder<> Builder(CI, /*FPMathTag=*/nullptr, OpBundles);
-  bool isCallingConvC = isCallingConvCCompatible(CI);
+
+  IRBuilderBase::OperandBundlesGuard Guard(Builder);
+  Builder.setDefaultOperandBundles(OpBundles);
 
   // Command-line parameter overrides instruction attribute.
   // This can't be moved to optimizeFloatingPointLibCall() because it may be
   // used by the intrinsic optimizations.
   if (EnableUnsafeFPShrink.getNumOccurrences() > 0)
     UnsafeFPShrink = EnableUnsafeFPShrink;
   else if (isa<FPMathOperator>(CI) && CI->isFast())
     UnsafeFPShrink = true;
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CI)) {
     case Intrinsic::memmove:
       return optimizeMemMove(CI, Builder);
     default:
       return nullptr;
     }
   }
 
   // Also try to simplify calls to fortified library functions.
-  if (Value *SimplifiedFortifiedCI = FortifiedSimplifier.optimizeCall(CI)) {
+  if (Value *SimplifiedFortifiedCI =
+          FortifiedSimplifier.optimizeCall(CI, Builder)) {
     // Try to further simplify the result.
     CallInst *SimplifiedCI = dyn_cast<CallInst>(SimplifiedFortifiedCI);
     if (SimplifiedCI && SimplifiedCI->getCalledFunction()) {
       // Ensure that SimplifiedCI's uses are complete, since some calls have
       // their uses analyzed.
       replaceAllUsesWith(CI, SimplifiedCI);
 
-      // Use an IR Builder from SimplifiedCI if available instead of CI
-      // to guarantee we reach all uses we might replace later on.
-      IRBuilder<> TmpBuilder(SimplifiedCI);
-      if (Value *V = optimizeStringMemoryLibCall(SimplifiedCI, TmpBuilder)) {
+      // Set insertion point to SimplifiedCI to guarantee we reach all uses
+      // we might replace later on.
+      IRBuilderBase::InsertPointGuard Guard(Builder);
+      Builder.SetInsertPoint(SimplifiedCI);
+      if (Value *V = optimizeStringMemoryLibCall(SimplifiedCI, Builder)) {
         // If we were able to further simplify, remove the now redundant call.
         substituteInParent(SimplifiedCI, V);
         return V;
       }
     }
     return SimplifiedFortifiedCI;
   }
 
Value *FortifiedLibCallSimplifier::optimizeVSPrintfChk(CallInst *CI,
                                                        IRBuilderBase &B) {
   if (isFortifiedCallFoldable(CI, 2, None, None, 1))
     return emitVSPrintf(CI->getArgOperand(0), CI->getArgOperand(3),
                         CI->getArgOperand(4), B, TLI);
 
   return nullptr;
 }
 
-Value *FortifiedLibCallSimplifier::optimizeCall(CallInst *CI) {
+Value *FortifiedLibCallSimplifier::optimizeCall(CallInst *CI,
+                                                IRBuilderBase &Builder) {
   // FIXME: We shouldn't be changing "nobuiltin" or TLI unavailable calls here.
   // Some clang users checked for _chk libcall availability using:
   //   __has_builtin(__builtin___memcpy_chk)
   // When compiling with -fno-builtin, this is always true.
   // When passing -ffreestanding/-mkernel, which both imply -fno-builtin, we
   // end up with fortified libcalls, which isn't acceptable in a freestanding
   // environment which only provides their non-fortified counterparts.
   //
   // Until we change clang and/or teach external users to check for availability
   // differently, disregard the "nobuiltin" attribute and TLI::has.
   //
   // PR23093.
 
   LibFunc Func;
   Function *Callee = CI->getCalledFunction();
+  bool isCallingConvC = isCallingConvCCompatible(CI);
 
   SmallVector<OperandBundleDef, 2> OpBundles;
   CI->getOperandBundlesAsDefs(OpBundles);
-  IRBuilder<> Builder(CI, /*FPMathTag=*/nullptr, OpBundles);
-  bool isCallingConvC = isCallingConvCCompatible(CI);
+
+  IRBuilderBase::OperandBundlesGuard Guard(Builder);
+  Builder.setDefaultOperandBundles(OpBundles);
 
   // First, check that this is a known library functions and that the prototype
   // is correct.
   if (!TLI->getLibFunc(*Callee, Func))
     return nullptr;
 
   // We never change the calling convention.
   if (!ignoreCallingConv(Func) && !isCallingConvC)

llvm/test/Transforms/InstCombine/simplify-libcalls.ll

-; RUN: opt -S < %s -instcombine | FileCheck %s
+; RUN: opt -S < %s -instcombine -instcombine-infinite-loop-threshold=2 | FileCheck %s
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-f80:128:128-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32-S32"
 
 @G = constant [3 x i8] c"%s\00"    ; <[3 x i8]*> [#uses=1]
 
 declare i32 @sprintf(i8*, i8*, ...)
 
 define void @foo(i8* %P, i32* %X) {
   call i32 (i8*, i8*, ...) @sprintf( i8* %P, i8* getelementptr ([3 x i8], [3 x i8]* @G, i32 0, i32 0), i32* %X )    ; <i32>:1 [#uses=0]