[InstCombine] Remove early constant fold

[nikic]

Summary

InstCombine currently performs a constant folding attempt as part of the main InstCombine loop, before visiting the instruction. However, each visit method will also attempt to simplify the instruction, which will in turn constant fold it. (Additionally, we also constant fold instructions before the main InstCombine loop and use a constant folding IR builder, so this is doubly redundant.)

There is one place where InstCombine visit methods currently don't call into simplification, and that's casts. To be conservative, I've added an explicit constant folding call there (though it has no impact on tests).

This makes for a mild compile-time improvement (http://llvm-compile-time-tracker.com/compare.php?from=b15d70e57a440c37c11b0ad3173d7dc624e4fa07&to=1753e2859139af9a8a8551780c791e53ad20f90d&stat=instructions:u) and in particular mitigates the compile-time regression from enabling load simplification in https://github.com/llvm/llvm-project/commit/be88b5814d9efce131dbc0c8e288907e2e6c89be.

[nikic]

llvm/test/Transforms/InstCombine/pr38677.ll

15

The reason for this change is that select constant folding in InstSimplify will only return if the select actually folds, not if it would create a constant expression. Arguably this is a bug in InstSimplify, but I decided not to fix it, since we don't actually want these select constant expressions.

[spatel]

LGTM - thanks!

[vitalybuka]

I am going to revert this one. Huge regression with ubsan or particular files.
I'll post reproducer here.

Flaky timeout on the bot https://lab.llvm.org/buildbot/#/builders/238 is from this.
But no need to look into bot yet, I believe I have -E reproducer.

[vitalybuka]

FYI @kstoimenov

[vitalybuka]

Original is on ARM https://gist.github.com/vitalybuka/82f6c5b2ab51ee027df13c2c35a95ffb

but I can reproduce compiling the one above on my default x86_64 checkout:

time <yourbuild>/bin/clang++ -fsanitize=undefined -Wl,--rpath=/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/libcxx_build_ubsan/lib -L/b/sanitizer-aarch64-linux-bootstrap-ubsan/build/libcxx_build_ubsan/lib -w -stdlib=libc++ -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-class-memaccess -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fno-omit-frame-pointer -gline-tables-only -fsanitize=undefined -fno-sanitize=vptr,function -fno-sanitize-recover=all -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -Wno-suggest-override -std=c++17 -o /dev/null -c t.cpp

[vitalybuka]

after revert:
real 3m51.948s
user 3m50.172s
sys 0m1.756s

before revert:
real 16m18.127s
user 16m16.255s
sys 0m1.801s

[vitalybuka]

On branch with ubsan does not show meaningful difference :(
https://llvm-compile-time-tracker.com/compare.php?from=a332a5159821071b71b0db09924952bb50b85033&to=2e565f60903052500d2a12f8e56a0efc7adc6f69&stat=instructions%3Au

[vitalybuka]

In D144369#4151125, @vitalybuka wrote:

On branch with ubsan does not show meaningful difference :(
https://llvm-compile-time-tracker.com/compare.php?from=a332a5159821071b71b0db09924952bb50b85033&to=2e565f60903052500d2a12f8e56a0efc7adc6f69&stat=instructions%3Au

it would be nice to compile clang itself as benchmark, but I guess it's significantly more load for tracker

[goldstein.w.n]

In D144369#4151551, @vitalybuka wrote:

In D144369#4151125, @vitalybuka wrote:

On branch with ubsan does not show meaningful difference :(
https://llvm-compile-time-tracker.com/compare.php?from=a332a5159821071b71b0db09924952bb50b85033&to=2e565f60903052500d2a12f8e56a0efc7adc6f69&stat=instructions%3Au

it would be nice to compile clang itself as benchmark, but I guess it's significantly more load for tracker

Could you just time the bootstrap tests and if they have an extreme regression fail the test?

[vitalybuka]

In D144369#4151553, @goldstein.w.n wrote:

Could you just time the bootstrap tests and if they have an extreme regression fail the test?

Thanks, looks just "time" is good enough. User time is quite consistent. Original version of the patch increased time ~20%, reapply is close to 0%
https://lab.llvm.org/buildbot/#/builders/238/builds/1576

llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp

InstCombinerImpl::isEliminableCastPair(const CastInst *CI1,
   return Instruction::CastOps(Res);
 }
 
 /// Implement the transforms common to all CastInst visitors.
 Instruction *InstCombinerImpl::commonCastTransforms(CastInst &CI) {
   Value *Src = CI.getOperand(0);
   Type *Ty = CI.getType();
 
+  if (auto *SrcC = dyn_cast<Constant>(Src))
+    if (Constant *Res = ConstantFoldCastOperand(CI.getOpcode(), SrcC, Ty, DL))
+      return replaceInstUsesWith(CI, Res);
+
   // Try to eliminate a cast of a cast.
   if (auto *CSrc = dyn_cast<CastInst>(Src)) {   // A->B->C cast
     if (Instruction::CastOps NewOpc = isEliminableCastPair(CSrc, &CI)) {
       // The first cast (CSrc) is eliminable so we need to fix up or replace
       // the second cast (CI). CSrc will then have a good chance of being dead.
       auto *Res = CastInst::Create(NewOpc, CSrc->getOperand(0), Ty);
       // Point debug users of the dying cast to the new one.
       if (CSrc->hasOneUse())
default:
     // TODO: Can handle more cases here.
     return false;
   }
 }
 
 Instruction *InstCombinerImpl::visitZExt(ZExtInst &Zext) {
   // If this zero extend is only used by a truncate, let the truncate be
   // eliminated before we try to optimize this zext.
-  if (Zext.hasOneUse() && isa<TruncInst>(Zext.user_back()))
+  if (Zext.hasOneUse() && isa<TruncInst>(Zext.user_back()) &&
+      !isa<Constant>(Zext.getOperand(0)))
     return nullptr;
 
   // If one of the common conversion will work, do it.
   if (Instruction *Result = commonCastTransforms(Zext))
     return Result;
 
   Value *Src = Zext.getOperand(0);
   Type *SrcTy = Src->getType(), *DestTy = Zext.getType();

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (isInstructionTriviallyDead(I, &TLI)) {
       eraseInstFromFunction(*I);
       ++NumDeadInst;
       continue;
     }
 
     if (!DebugCounter::shouldExecute(VisitCounter))
       continue;
 
-    // Instruction isn't dead, see if we can constant propagate it.
-    if (!I->use_empty() &&
-        (I->getNumOperands() == 0 || isa<Constant>(I->getOperand(0)))) {
-      if (Constant *C = ConstantFoldInstruction(I, DL, &TLI)) {
-        LLVM_DEBUG(dbgs() << "IC: ConstFold to: " << *C << " from: " << *I
-                          << '\n');
-
-        // Add operands to the worklist.
-        replaceInstUsesWith(*I, C);
-        ++NumConstProp;
-        if (isInstructionTriviallyDead(I, &TLI))
-          eraseInstFromFunction(*I);
-        MadeIRChange = true;
-        continue;
-      }
-    }
-
     // See if we can trivially sink this instruction to its user if we can
     // prove that the successor is not executed more frequently than our block.
     // Return the UserBlock if successful.
     auto getOptionalSinkBlockForInst =
         [this](Instruction *I) -> std::optional<BasicBlock *> {
       if (!EnableCodeSinking)
         return std::nullopt;
 

llvm/test/Transforms/InstCombine/pr38677.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ;RUN: opt -passes=instcombine -S %s | FileCheck %s
 
 @A = extern_weak global i32, align 4
 @B = extern_weak global i32, align 4
 
 define i32 @foo(i1 %which, ptr %dst) {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    br i1 true, label [[FINAL:%.*]], label [[DELAY:%.*]]
 ; CHECK:       delay:
 ; CHECK-NEXT:    br label [[FINAL]]
 ; CHECK:       final:
-; CHECK-NEXT:    [[USE2:%.*]] = phi i32 [ 1, [[ENTRY:%.*]] ], [ select (i1 icmp eq (ptr @A, ptr @B), i32 2, i32 1), [[DELAY]] ]
 ; CHECK-NEXT:    store i1 false, ptr [[DST:%.*]], align 1
-; CHECK-NEXT:    ret i32 [[USE2]]
+; CHECK-NEXT:    ret i32 1
 ;
 entry:
   br i1 true, label %final, label %delay
 
 delay:                                            ; preds = %entry
   br label %final
 
 final:                                            ; preds = %delay, %entry
   %use2 = phi i1 [ false, %entry ], [ icmp eq (ptr @A, ptr @B), %delay ]
   %value = select i1 %use2, i32 2, i32 1
   %B7 = mul i32 %value, 2147483647
   %C3 = icmp ule i32 %B7, 0
   store i1 %C3, ptr %dst
   ret i32 %value
 }