From bab200ac44b0a627378b48e6dedd40d6e4b4f798 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 21 Jul 2021 21:23:38 +0200 Subject: [PATCH] [IR] Consider non-willreturn as side effect (PR50511) This adjusts mayHaveSideEffect() to return true for !willReturn() instructions. Just like other side-effects, non-willreturn calls (aka "divergence") cannot be removed and cannot be reordered relative to other side effects. This fixes a number of bugs where non-willreturn calls are either incorrectly dropped or moved. In particular, it also fixes the last open problem in https://bugs.llvm.org/show_bug.cgi?id=50511. I performed a cursory review of all current mayHaveSideEffect() uses, which convinced me that these are indeed the desired default semantics. Places that do not want to consider non-willreturn as a sideeffect generally do not want mayHaveSideEffect() semantics at all. I identified two such cases, which are addressed by D106591 and D106742. Finally, there is a use in SCEV for which we don't really have an appropriate API right now -- what it wants is basically "would this be considered forward progress". I've just spelled out the previous semantics there. Differential Revision: https://reviews.llvm.org/D106749 --- include/llvm/IR/Instruction.h | 7 ++++++- lib/Analysis/DemandedBits.cpp | 2 +- lib/Analysis/ScalarEvolution.cpp | 2 +- lib/IR/Instruction.cpp | 4 ++++ lib/Transforms/Scalar/ADCE.cpp | 2 +- test/Transforms/LICM/sinking.ll | 6 +++--- test/Transforms/LoopDeletion/noop-loops-with-subloops.ll | 9 +++++++-- test/Transforms/SCCP/calltest.ll | 2 +- 8 files changed, 24 insertions(+), 10 deletions(-) diff --git a/include/llvm/IR/Instruction.h b/include/llvm/IR/Instruction.h index 391fb32c9ca..5ca9c69268e 100644 --- a/include/llvm/IR/Instruction.h +++ b/include/llvm/IR/Instruction.h @@ -627,11 +627,16 @@ public: /// Return true if the instruction may have side effects. /// + /// Side effects are: + /// * Writing to memory. + /// * Unwinding. + /// * Not returning (e.g. an infinite loop). + /// /// Note that this does not consider malloc and alloca to have side /// effects because the newly allocated memory is completely invisible to /// instructions which don't use the returned value. For cases where this /// matters, isSafeToSpeculativelyExecute may be more appropriate. - bool mayHaveSideEffects() const { return mayWriteToMemory() || mayThrow(); } + bool mayHaveSideEffects() const; /// Return true if the instruction can be removed if the result is unused. /// diff --git a/lib/Analysis/DemandedBits.cpp b/lib/Analysis/DemandedBits.cpp index 467dea9c023..ca6d58fac82 100644 --- a/lib/Analysis/DemandedBits.cpp +++ b/lib/Analysis/DemandedBits.cpp @@ -80,7 +80,7 @@ void DemandedBitsWrapperPass::print(raw_ostream &OS, const Module *M) const { static bool isAlwaysLive(Instruction *I) { return I->isTerminator() || isa(I) || I->isEHPad() || - I->mayHaveSideEffects() || !I->willReturn(); + I->mayHaveSideEffects(); } void DemandedBits::determineLiveOperandBits( diff --git a/lib/Analysis/ScalarEvolution.cpp b/lib/Analysis/ScalarEvolution.cpp index 3a1182ca01e..df38a850c28 100644 --- a/lib/Analysis/ScalarEvolution.cpp +++ b/lib/Analysis/ScalarEvolution.cpp @@ -6677,7 +6677,7 @@ ScalarEvolution::getLoopProperties(const Loop *L) { if (auto *SI = dyn_cast(I)) return !SI->isSimple(); - return I->mayHaveSideEffects(); + return I->mayThrow() || I->mayWriteToMemory(); }; LoopProperties LP = {/* HasNoAbnormalExits */ true, diff --git a/lib/IR/Instruction.cpp b/lib/IR/Instruction.cpp index a6fba2a7a24..649a85e6a4c 100644 --- a/lib/IR/Instruction.cpp +++ b/lib/IR/Instruction.cpp @@ -663,6 +663,10 @@ bool Instruction::mayThrow() const { return isa(this); } +bool Instruction::mayHaveSideEffects() const { + return mayWriteToMemory() || mayThrow() || !willReturn(); +} + bool Instruction::isSafeToRemove() const { return (!isa(this) || !this->mayHaveSideEffects()) && !this->isTerminator(); diff --git a/lib/Transforms/Scalar/ADCE.cpp b/lib/Transforms/Scalar/ADCE.cpp index 12a3e678393..6f3fdb88eda 100644 --- a/lib/Transforms/Scalar/ADCE.cpp +++ b/lib/Transforms/Scalar/ADCE.cpp @@ -326,7 +326,7 @@ void AggressiveDeadCodeElimination::initialize() { bool AggressiveDeadCodeElimination::isAlwaysLive(Instruction &I) { // TODO -- use llvm::isInstructionTriviallyDead - if (I.isEHPad() || I.mayHaveSideEffects() || !I.willReturn()) { + if (I.isEHPad() || I.mayHaveSideEffects()) { // Skip any value profile instrumentation calls if they are // instrumenting constants. if (isInstrumentsConstant(I)) diff --git a/test/Transforms/LICM/sinking.ll b/test/Transforms/LICM/sinking.ll index ee7f6338e6a..e8660695aa7 100644 --- a/test/Transforms/LICM/sinking.ll +++ b/test/Transforms/LICM/sinking.ll @@ -979,16 +979,16 @@ try.cont: ret void } -; TODO: Should not get sunk. define i32 @not_willreturn(i8* %p) { ; CHECK-LABEL: @not_willreturn( +; CHECK-NEXT: [[X:%.*]] = call i32 @getv() #[[ATTR5:[0-9]+]] ; CHECK-NEXT: br label [[LOOP:%.*]] ; CHECK: loop: ; CHECK-NEXT: store volatile i8 0, i8* [[P:%.*]], align 1 ; CHECK-NEXT: br i1 true, label [[LOOP]], label [[OUT:%.*]] ; CHECK: out: -; CHECK-NEXT: [[X_LE:%.*]] = call i32 @getv() #[[ATTR5:[0-9]+]] -; CHECK-NEXT: ret i32 [[X_LE]] +; CHECK-NEXT: [[X_LCSSA:%.*]] = phi i32 [ [[X]], [[LOOP]] ] +; CHECK-NEXT: ret i32 [[X_LCSSA]] ; br label %loop diff --git a/test/Transforms/LoopDeletion/noop-loops-with-subloops.ll b/test/Transforms/LoopDeletion/noop-loops-with-subloops.ll index 9163c9cd401..bf5288085df 100644 --- a/test/Transforms/LoopDeletion/noop-loops-with-subloops.ll +++ b/test/Transforms/LoopDeletion/noop-loops-with-subloops.ll @@ -395,11 +395,16 @@ exit: } ; Inner infinite loop hidden behind a call. -; TODO: Loop should not get deleted. define void @not_willreturn() { ; CHECK-LABEL: @not_willreturn( ; CHECK-NEXT: entry: -; CHECK-NEXT: br label [[EXIT:%.*]] +; CHECK-NEXT: br label [[LOOP:%.*]] +; CHECK: loop: +; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP]] ] +; CHECK-NEXT: call void @sideeffect() #[[ATTR2:[0-9]+]] +; CHECK-NEXT: [[IV_NEXT]] = add nuw i32 [[IV]], 1 +; CHECK-NEXT: [[C:%.*]] = icmp ult i32 [[IV]], 100 +; CHECK-NEXT: br i1 [[C]], label [[LOOP]], label [[EXIT:%.*]] ; CHECK: exit: ; CHECK-NEXT: ret void ; diff --git a/test/Transforms/SCCP/calltest.ll b/test/Transforms/SCCP/calltest.ll index de793635600..2a6445fcb68 100644 --- a/test/Transforms/SCCP/calltest.ll +++ b/test/Transforms/SCCP/calltest.ll @@ -34,9 +34,9 @@ define i32 @test_1() { ret i32 0 } -; TODO: Call should not get dropped. define i32 @test_not_willreturn() { ; CHECK-LABEL: @test_not_willreturn( +; CHECK-NEXT: [[TMP1:%.*]] = call [[EMPTY:%.*]] @has_side_effects() #[[ATTR1:[0-9]+]] ; CHECK-NEXT: ret i32 0 ; %1 = call %empty @has_side_effects() nounwind readonly