-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Revert "[LoopInfo] Pointer to stack object may not be loop invariant in a coroutine function (#149936)" #157986
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Weibo He (NewSigma) ChangesSince #156788 has resolved #149604, maybe we can revert this workaround now. Close #149604 Full diff: https://github.com/llvm/llvm-project/pull/157986.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Analysis/LoopInfo.h b/llvm/include/llvm/Analysis/LoopInfo.h
index f80744e70f7ad..a7a6a2753709c 100644
--- a/llvm/include/llvm/Analysis/LoopInfo.h
+++ b/llvm/include/llvm/Analysis/LoopInfo.h
@@ -59,12 +59,11 @@ class LLVM_ABI Loop : public LoopBase<BasicBlock, Loop> {
};
/// Return true if the specified value is loop invariant.
- bool isLoopInvariant(const Value *V, bool HasCoroSuspendInst = false) const;
+ bool isLoopInvariant(const Value *V) const;
/// Return true if all the operands of the specified instruction are loop
/// invariant.
- bool hasLoopInvariantOperands(const Instruction *I,
- bool HasCoroSuspendInst = false) const;
+ bool hasLoopInvariantOperands(const Instruction *I) const;
/// If the given value is an instruction inside of the loop and it can be
/// hoisted, do so to make it trivially loop-invariant.
diff --git a/llvm/include/llvm/Transforms/Utils/LoopUtils.h b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
index 96e3d3d47f2d0..490bff0589c09 100644
--- a/llvm/include/llvm/Transforms/Utils/LoopUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
@@ -185,8 +185,7 @@ LLVM_ABI bool hoistRegion(DomTreeNode *, AAResults *, LoopInfo *,
TargetLibraryInfo *, Loop *, MemorySSAUpdater &,
ScalarEvolution *, ICFLoopSafetyInfo *,
SinkAndHoistLICMFlags &, OptimizationRemarkEmitter *,
- bool, bool AllowSpeculation,
- bool HasCoroSuspendInst = false);
+ bool, bool AllowSpeculation);
/// Return true if the induction variable \p IV in a Loop whose latch is
/// \p LatchBlock would become dead if the exit test \p Cond were removed.
diff --git a/llvm/lib/Analysis/LoopInfo.cpp b/llvm/lib/Analysis/LoopInfo.cpp
index 6ba6073cce950..a8c3173bb1794 100644
--- a/llvm/lib/Analysis/LoopInfo.cpp
+++ b/llvm/lib/Analysis/LoopInfo.cpp
@@ -58,26 +58,14 @@ static cl::opt<bool, true>
// Loop implementation
//
-bool Loop::isLoopInvariant(const Value *V, bool HasCoroSuspendInst) const {
- if (const Instruction *I = dyn_cast<Instruction>(V)) {
- // FIXME: this is semantically inconsistent. We're tracking a proper fix in
- // issue #149604.
- // If V is a pointer to stack object and L contains a coro.suspend function
- // call, then V may not be loop invariant because the ramp function and
- // resume function have different stack frames.
- if (HasCoroSuspendInst && isa<AllocaInst>(I))
- return false;
- else
- return !contains(I);
- }
+bool Loop::isLoopInvariant(const Value *V) const {
+ if (const Instruction *I = dyn_cast<Instruction>(V))
+ return !contains(I);
return true; // All non-instructions are loop invariant
}
-bool Loop::hasLoopInvariantOperands(const Instruction *I,
- bool HasCoroSuspendInst) const {
- return all_of(I->operands(), [&](Value *V) {
- return isLoopInvariant(V, HasCoroSuspendInst);
- });
+bool Loop::hasLoopInvariantOperands(const Instruction *I) const {
+ return all_of(I->operands(), [&](Value *V) { return isLoopInvariant(V); });
}
bool Loop::makeLoopInvariant(Value *V, bool &Changed, Instruction *InsertPt,
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index e157cc9212769..40104e8fb4249 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -474,7 +474,7 @@ bool LoopInvariantCodeMotion::runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI,
if (Preheader)
Changed |= hoistRegion(DT->getNode(L->getHeader()), AA, LI, DT, AC, TLI, L,
MSSAU, SE, &SafetyInfo, Flags, ORE, LoopNestMode,
- LicmAllowSpeculation, HasCoroSuspendInst);
+ LicmAllowSpeculation);
// Now that all loop invariants have been removed from the loop, promote any
// memory references to scalars that we can.
@@ -892,7 +892,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
ICFLoopSafetyInfo *SafetyInfo,
SinkAndHoistLICMFlags &Flags,
OptimizationRemarkEmitter *ORE, bool LoopNestMode,
- bool AllowSpeculation, bool HasCoroSuspendInst) {
+ bool AllowSpeculation) {
// Verify inputs.
assert(N != nullptr && AA != nullptr && LI != nullptr && DT != nullptr &&
CurLoop != nullptr && SafetyInfo != nullptr &&
@@ -925,7 +925,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
// TODO: It may be safe to hoist if we are hoisting to a conditional block
// and we have accurately duplicated the control flow from the loop header
// to that block.
- if (CurLoop->hasLoopInvariantOperands(&I, HasCoroSuspendInst) &&
+ if (CurLoop->hasLoopInvariantOperands(&I) &&
canSinkOrHoistInst(I, AA, DT, CurLoop, MSSAU, true, Flags, ORE) &&
isSafeToExecuteUnconditionally(I, DT, TLI, CurLoop, SafetyInfo, ORE,
Preheader->getTerminator(), AC,
@@ -975,7 +975,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
SafetyInfo->doesNotWriteMemoryBefore(I, CurLoop);
};
if ((IsInvariantStart(I) || isGuard(&I)) &&
- CurLoop->hasLoopInvariantOperands(&I, HasCoroSuspendInst) &&
+ CurLoop->hasLoopInvariantOperands(&I) &&
MustExecuteWithoutWritesBefore(I)) {
hoist(I, DT, CurLoop, CFH.getOrCreateHoistedBlock(BB), SafetyInfo,
MSSAU, SE, ORE);
diff --git a/llvm/test/Transforms/LICM/licm-coroutine.ll b/llvm/test/Transforms/LICM/licm-coroutine.ll
deleted file mode 100644
index a4765acfb93f8..0000000000000
--- a/llvm/test/Transforms/LICM/licm-coroutine.ll
+++ /dev/null
@@ -1,78 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt < %s -passes=licm -S | FileCheck %s
-
-; %fca.0 and %fca.1 should not be hoisted out of the loop because the ramp
-; function and resume function have different stack frames, so %pointer1 and
-; %pointer2 have different values before and after @llvm.coro.suspend.
-
-define ptr @f(i32 %n) presplitcoroutine {
-; CHECK-LABEL: define ptr @f(
-; CHECK-SAME: i32 [[N:%.*]]) #[[ATTR0:[0-9]+]] {
-; CHECK-NEXT: [[ENTRY:.*]]:
-; CHECK-NEXT: [[POINTER1:%.*]] = alloca ptr, align 8
-; CHECK-NEXT: [[POINTER2:%.*]] = alloca ptr, align 8
-; CHECK-NEXT: [[ID:%.*]] = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
-; CHECK-NEXT: [[SIZE:%.*]] = call i32 @llvm.coro.size.i32()
-; CHECK-NEXT: [[ALLOC:%.*]] = call ptr @malloc(i32 [[SIZE]])
-; CHECK-NEXT: [[HDL:%.*]] = call noalias ptr @llvm.coro.begin(token [[ID]], ptr [[ALLOC]])
-; CHECK-NEXT: br label %[[LOOP:.*]]
-; CHECK: [[LOOP]]:
-; CHECK-NEXT: [[N_VAL:%.*]] = phi i32 [ [[N]], %[[ENTRY]] ], [ [[INC:%.*]], %[[RESUME:.*]] ]
-; CHECK-NEXT: [[INC]] = add nsw i32 [[N_VAL]], 1
-; CHECK-NEXT: call void @print(i32 [[N_VAL]])
-; CHECK-NEXT: [[TMP0:%.*]] = call i8 @llvm.coro.suspend(token none, i1 false)
-; CHECK-NEXT: switch i8 [[TMP0]], label %[[SUSPEND_LOOPEXIT:.*]] [
-; CHECK-NEXT: i8 0, label %[[RESUME]]
-; CHECK-NEXT: i8 1, label %[[CLEANUP:.*]]
-; CHECK-NEXT: ]
-; CHECK: [[RESUME]]:
-; CHECK-NEXT: [[FCA_0:%.*]] = insertvalue [2 x ptr] poison, ptr [[POINTER1]], 0
-; CHECK-NEXT: [[FCA_1:%.*]] = insertvalue [2 x ptr] [[FCA_0]], ptr [[POINTER2]], 1
-; CHECK-NEXT: call void @foo([2 x ptr] [[FCA_1]])
-; CHECK-NEXT: br label %[[LOOP]]
-; CHECK: [[CLEANUP]]:
-; CHECK-NEXT: [[MEM:%.*]] = call ptr @llvm.coro.free(token [[ID]], ptr [[HDL]])
-; CHECK-NEXT: call void @free(ptr [[MEM]])
-; CHECK-NEXT: br label %[[SUSPEND:.*]]
-; CHECK: [[SUSPEND_LOOPEXIT]]:
-; CHECK-NEXT: br label %[[SUSPEND]]
-; CHECK: [[SUSPEND]]:
-; CHECK-NEXT: [[UNUSED:%.*]] = call i1 @llvm.coro.end(ptr [[HDL]], i1 false, token none)
-; CHECK-NEXT: ret ptr [[HDL]]
-;
-entry:
- %pointer1 = alloca ptr
- %pointer2 = alloca ptr
- %id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
- %size = call i32 @llvm.coro.size.i32()
- %alloc = call ptr @malloc(i32 %size)
- %hdl = call noalias ptr @llvm.coro.begin(token %id, ptr %alloc)
- br label %loop
-
-loop:
- %n.val = phi i32 [ %n, %entry ], [ %inc, %resume ]
- %inc = add nsw i32 %n.val, 1
- call void @print(i32 %n.val)
- %0 = call i8 @llvm.coro.suspend(token none, i1 false)
- switch i8 %0, label %suspend [i8 0, label %resume
- i8 1, label %cleanup]
-
-resume:
- %fca.0 = insertvalue [2 x ptr] poison, ptr %pointer1, 0
- %fca.1 = insertvalue [2 x ptr] %fca.0, ptr %pointer2, 1
- call void @foo([2 x ptr] %fca.1)
- br label %loop
-
-cleanup:
- %mem = call ptr @llvm.coro.free(token %id, ptr %hdl)
- call void @free(ptr %mem)
- br label %suspend
-suspend:
- %unused = call i1 @llvm.coro.end(ptr %hdl, i1 false, token none)
- ret ptr %hdl
-}
-
-declare void @free(ptr)
-declare ptr @malloc(i32)
-declare void @print(i32)
-declare void @foo([2 x ptr])
|
@llvm/pr-subscribers-llvm-analysis Author: Weibo He (NewSigma) ChangesSince #156788 has resolved #149604, maybe we can revert this workaround now. Close #149604 Full diff: https://github.com/llvm/llvm-project/pull/157986.diff 5 Files Affected:
diff --git a/llvm/include/llvm/Analysis/LoopInfo.h b/llvm/include/llvm/Analysis/LoopInfo.h
index f80744e70f7ad..a7a6a2753709c 100644
--- a/llvm/include/llvm/Analysis/LoopInfo.h
+++ b/llvm/include/llvm/Analysis/LoopInfo.h
@@ -59,12 +59,11 @@ class LLVM_ABI Loop : public LoopBase<BasicBlock, Loop> {
};
/// Return true if the specified value is loop invariant.
- bool isLoopInvariant(const Value *V, bool HasCoroSuspendInst = false) const;
+ bool isLoopInvariant(const Value *V) const;
/// Return true if all the operands of the specified instruction are loop
/// invariant.
- bool hasLoopInvariantOperands(const Instruction *I,
- bool HasCoroSuspendInst = false) const;
+ bool hasLoopInvariantOperands(const Instruction *I) const;
/// If the given value is an instruction inside of the loop and it can be
/// hoisted, do so to make it trivially loop-invariant.
diff --git a/llvm/include/llvm/Transforms/Utils/LoopUtils.h b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
index 96e3d3d47f2d0..490bff0589c09 100644
--- a/llvm/include/llvm/Transforms/Utils/LoopUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/LoopUtils.h
@@ -185,8 +185,7 @@ LLVM_ABI bool hoistRegion(DomTreeNode *, AAResults *, LoopInfo *,
TargetLibraryInfo *, Loop *, MemorySSAUpdater &,
ScalarEvolution *, ICFLoopSafetyInfo *,
SinkAndHoistLICMFlags &, OptimizationRemarkEmitter *,
- bool, bool AllowSpeculation,
- bool HasCoroSuspendInst = false);
+ bool, bool AllowSpeculation);
/// Return true if the induction variable \p IV in a Loop whose latch is
/// \p LatchBlock would become dead if the exit test \p Cond were removed.
diff --git a/llvm/lib/Analysis/LoopInfo.cpp b/llvm/lib/Analysis/LoopInfo.cpp
index 6ba6073cce950..a8c3173bb1794 100644
--- a/llvm/lib/Analysis/LoopInfo.cpp
+++ b/llvm/lib/Analysis/LoopInfo.cpp
@@ -58,26 +58,14 @@ static cl::opt<bool, true>
// Loop implementation
//
-bool Loop::isLoopInvariant(const Value *V, bool HasCoroSuspendInst) const {
- if (const Instruction *I = dyn_cast<Instruction>(V)) {
- // FIXME: this is semantically inconsistent. We're tracking a proper fix in
- // issue #149604.
- // If V is a pointer to stack object and L contains a coro.suspend function
- // call, then V may not be loop invariant because the ramp function and
- // resume function have different stack frames.
- if (HasCoroSuspendInst && isa<AllocaInst>(I))
- return false;
- else
- return !contains(I);
- }
+bool Loop::isLoopInvariant(const Value *V) const {
+ if (const Instruction *I = dyn_cast<Instruction>(V))
+ return !contains(I);
return true; // All non-instructions are loop invariant
}
-bool Loop::hasLoopInvariantOperands(const Instruction *I,
- bool HasCoroSuspendInst) const {
- return all_of(I->operands(), [&](Value *V) {
- return isLoopInvariant(V, HasCoroSuspendInst);
- });
+bool Loop::hasLoopInvariantOperands(const Instruction *I) const {
+ return all_of(I->operands(), [&](Value *V) { return isLoopInvariant(V); });
}
bool Loop::makeLoopInvariant(Value *V, bool &Changed, Instruction *InsertPt,
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index e157cc9212769..40104e8fb4249 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -474,7 +474,7 @@ bool LoopInvariantCodeMotion::runOnLoop(Loop *L, AAResults *AA, LoopInfo *LI,
if (Preheader)
Changed |= hoistRegion(DT->getNode(L->getHeader()), AA, LI, DT, AC, TLI, L,
MSSAU, SE, &SafetyInfo, Flags, ORE, LoopNestMode,
- LicmAllowSpeculation, HasCoroSuspendInst);
+ LicmAllowSpeculation);
// Now that all loop invariants have been removed from the loop, promote any
// memory references to scalars that we can.
@@ -892,7 +892,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
ICFLoopSafetyInfo *SafetyInfo,
SinkAndHoistLICMFlags &Flags,
OptimizationRemarkEmitter *ORE, bool LoopNestMode,
- bool AllowSpeculation, bool HasCoroSuspendInst) {
+ bool AllowSpeculation) {
// Verify inputs.
assert(N != nullptr && AA != nullptr && LI != nullptr && DT != nullptr &&
CurLoop != nullptr && SafetyInfo != nullptr &&
@@ -925,7 +925,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
// TODO: It may be safe to hoist if we are hoisting to a conditional block
// and we have accurately duplicated the control flow from the loop header
// to that block.
- if (CurLoop->hasLoopInvariantOperands(&I, HasCoroSuspendInst) &&
+ if (CurLoop->hasLoopInvariantOperands(&I) &&
canSinkOrHoistInst(I, AA, DT, CurLoop, MSSAU, true, Flags, ORE) &&
isSafeToExecuteUnconditionally(I, DT, TLI, CurLoop, SafetyInfo, ORE,
Preheader->getTerminator(), AC,
@@ -975,7 +975,7 @@ bool llvm::hoistRegion(DomTreeNode *N, AAResults *AA, LoopInfo *LI,
SafetyInfo->doesNotWriteMemoryBefore(I, CurLoop);
};
if ((IsInvariantStart(I) || isGuard(&I)) &&
- CurLoop->hasLoopInvariantOperands(&I, HasCoroSuspendInst) &&
+ CurLoop->hasLoopInvariantOperands(&I) &&
MustExecuteWithoutWritesBefore(I)) {
hoist(I, DT, CurLoop, CFH.getOrCreateHoistedBlock(BB), SafetyInfo,
MSSAU, SE, ORE);
diff --git a/llvm/test/Transforms/LICM/licm-coroutine.ll b/llvm/test/Transforms/LICM/licm-coroutine.ll
deleted file mode 100644
index a4765acfb93f8..0000000000000
--- a/llvm/test/Transforms/LICM/licm-coroutine.ll
+++ /dev/null
@@ -1,78 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
-; RUN: opt < %s -passes=licm -S | FileCheck %s
-
-; %fca.0 and %fca.1 should not be hoisted out of the loop because the ramp
-; function and resume function have different stack frames, so %pointer1 and
-; %pointer2 have different values before and after @llvm.coro.suspend.
-
-define ptr @f(i32 %n) presplitcoroutine {
-; CHECK-LABEL: define ptr @f(
-; CHECK-SAME: i32 [[N:%.*]]) #[[ATTR0:[0-9]+]] {
-; CHECK-NEXT: [[ENTRY:.*]]:
-; CHECK-NEXT: [[POINTER1:%.*]] = alloca ptr, align 8
-; CHECK-NEXT: [[POINTER2:%.*]] = alloca ptr, align 8
-; CHECK-NEXT: [[ID:%.*]] = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
-; CHECK-NEXT: [[SIZE:%.*]] = call i32 @llvm.coro.size.i32()
-; CHECK-NEXT: [[ALLOC:%.*]] = call ptr @malloc(i32 [[SIZE]])
-; CHECK-NEXT: [[HDL:%.*]] = call noalias ptr @llvm.coro.begin(token [[ID]], ptr [[ALLOC]])
-; CHECK-NEXT: br label %[[LOOP:.*]]
-; CHECK: [[LOOP]]:
-; CHECK-NEXT: [[N_VAL:%.*]] = phi i32 [ [[N]], %[[ENTRY]] ], [ [[INC:%.*]], %[[RESUME:.*]] ]
-; CHECK-NEXT: [[INC]] = add nsw i32 [[N_VAL]], 1
-; CHECK-NEXT: call void @print(i32 [[N_VAL]])
-; CHECK-NEXT: [[TMP0:%.*]] = call i8 @llvm.coro.suspend(token none, i1 false)
-; CHECK-NEXT: switch i8 [[TMP0]], label %[[SUSPEND_LOOPEXIT:.*]] [
-; CHECK-NEXT: i8 0, label %[[RESUME]]
-; CHECK-NEXT: i8 1, label %[[CLEANUP:.*]]
-; CHECK-NEXT: ]
-; CHECK: [[RESUME]]:
-; CHECK-NEXT: [[FCA_0:%.*]] = insertvalue [2 x ptr] poison, ptr [[POINTER1]], 0
-; CHECK-NEXT: [[FCA_1:%.*]] = insertvalue [2 x ptr] [[FCA_0]], ptr [[POINTER2]], 1
-; CHECK-NEXT: call void @foo([2 x ptr] [[FCA_1]])
-; CHECK-NEXT: br label %[[LOOP]]
-; CHECK: [[CLEANUP]]:
-; CHECK-NEXT: [[MEM:%.*]] = call ptr @llvm.coro.free(token [[ID]], ptr [[HDL]])
-; CHECK-NEXT: call void @free(ptr [[MEM]])
-; CHECK-NEXT: br label %[[SUSPEND:.*]]
-; CHECK: [[SUSPEND_LOOPEXIT]]:
-; CHECK-NEXT: br label %[[SUSPEND]]
-; CHECK: [[SUSPEND]]:
-; CHECK-NEXT: [[UNUSED:%.*]] = call i1 @llvm.coro.end(ptr [[HDL]], i1 false, token none)
-; CHECK-NEXT: ret ptr [[HDL]]
-;
-entry:
- %pointer1 = alloca ptr
- %pointer2 = alloca ptr
- %id = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
- %size = call i32 @llvm.coro.size.i32()
- %alloc = call ptr @malloc(i32 %size)
- %hdl = call noalias ptr @llvm.coro.begin(token %id, ptr %alloc)
- br label %loop
-
-loop:
- %n.val = phi i32 [ %n, %entry ], [ %inc, %resume ]
- %inc = add nsw i32 %n.val, 1
- call void @print(i32 %n.val)
- %0 = call i8 @llvm.coro.suspend(token none, i1 false)
- switch i8 %0, label %suspend [i8 0, label %resume
- i8 1, label %cleanup]
-
-resume:
- %fca.0 = insertvalue [2 x ptr] poison, ptr %pointer1, 0
- %fca.1 = insertvalue [2 x ptr] %fca.0, ptr %pointer2, 1
- call void @foo([2 x ptr] %fca.1)
- br label %loop
-
-cleanup:
- %mem = call ptr @llvm.coro.free(token %id, ptr %hdl)
- call void @free(ptr %mem)
- br label %suspend
-suspend:
- %unused = call i1 @llvm.coro.end(ptr %hdl, i1 false, token none)
- ret ptr %hdl
-}
-
-declare void @free(ptr)
-declare ptr @malloc(i32)
-declare void @print(i32)
-declare void @foo([2 x ptr])
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
Since #156788 has resolved #149604, maybe we can revert this workaround now.
Close #149604