[llvm] 5675757 - [GuardUtils] Add asserts about loop varying widenable conditions
Anna Thomas via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 11 07:54:19 PDT 2023
Author: Anna Thomas
Date: 2023-04-11T10:54:07-04:00
New Revision: 5675757f5fc6e27ce01b3b12bdfd04044df53aa3
URL: https://github.com/llvm/llvm-project/commit/5675757f5fc6e27ce01b3b12bdfd04044df53aa3
DIFF: https://github.com/llvm/llvm-project/commit/5675757f5fc6e27ce01b3b12bdfd04044df53aa3.diff
LOG: [GuardUtils] Add asserts about loop varying widenable conditions
We have now seen two miscompiles because of widening widenable
conditions at incorrect IR points and thereby changing a branch's loop
invariant condition to a loop-varying one (see PR60234 and PR61963).
This patch adds asserts in common guard utilities that we use for
widening to proactively catch these bugs in future.
Note that these asserts will not fire if we were to sink a widenable
condition from out of a loop into a loop (that's also incorrect for the
same reason as above).
Tested this without the fix for PR60234 (guard widening miscompile) and
confirmed the assert fires.
WARNING: Sometimes, the assert can fire if we failed to hoist the
invariant condition out of the loop. This is a pass-ordering issue or a
limitation in LICM, which would need an investigation. See details in
review.
Differential Revision: https://reviews.llvm.org/D147752
Added:
Modified:
llvm/include/llvm/Transforms/Utils/GuardUtils.h
llvm/lib/Transforms/Scalar/GuardWidening.cpp
llvm/lib/Transforms/Scalar/LoopPredication.cpp
llvm/lib/Transforms/Utils/GuardUtils.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Transforms/Utils/GuardUtils.h b/llvm/include/llvm/Transforms/Utils/GuardUtils.h
index 7ab5d9ef4f238..c4e5db4b66423 100644
--- a/llvm/include/llvm/Transforms/Utils/GuardUtils.h
+++ b/llvm/include/llvm/Transforms/Utils/GuardUtils.h
@@ -17,6 +17,7 @@ namespace llvm {
class BranchInst;
class CallInst;
class Function;
+class LoopInfo;
class Value;
/// Splits control flow at point of \p Guard, replacing it with explicit branch
@@ -32,12 +33,14 @@ void makeGuardControlFlowExplicit(Function *DeoptIntrinsic, CallInst *Guard,
/// Given a branch we know is widenable (defined per Analysis/GuardUtils.h),
/// widen it such that condition 'NewCond' is also known to hold on the taken
/// path. Branch remains widenable after transform.
-void widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond);
+void widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond,
+ const LoopInfo &LI);
/// Given a branch we know is widenable (defined per Analysis/GuardUtils.h),
/// *set* it's condition such that (only) 'Cond' is known to hold on the taken
/// path and that the branch remains widenable after transform.
-void setWidenableBranchCond(BranchInst *WidenableBR, Value *Cond);
+void setWidenableBranchCond(BranchInst *WidenableBR, Value *Cond,
+ const LoopInfo &LI);
} // llvm
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index 62b40a23e38c0..c7c5921b781b5 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -289,7 +289,7 @@ class GuardWideningImpl {
widenCondCommon(getCondition(ToWiden), NewCondition, InsertPt, Result,
InvertCondition);
if (isGuardAsWidenableBranch(ToWiden)) {
- setWidenableBranchCond(cast<BranchInst>(ToWiden), Result);
+ setWidenableBranchCond(cast<BranchInst>(ToWiden), Result, LI);
return;
}
setCondition(ToWiden, Result);
diff --git a/llvm/lib/Transforms/Scalar/LoopPredication.cpp b/llvm/lib/Transforms/Scalar/LoopPredication.cpp
index 12852ae5c4608..78d3991d166ea 100644
--- a/llvm/lib/Transforms/Scalar/LoopPredication.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopPredication.cpp
@@ -1248,7 +1248,7 @@ bool LoopPredication::predicateLoopExits(Loop *L, SCEVExpander &Rewriter) {
// context.
NewCond = B.CreateFreeze(NewCond);
- widenWidenableBranch(WidenableBR, NewCond);
+ widenWidenableBranch(WidenableBR, NewCond, *LI);
Value *OldCond = BI->getCondition();
BI->setCondition(ConstantInt::get(OldCond->getType(), !ExitIfTrue));
diff --git a/llvm/lib/Transforms/Utils/GuardUtils.cpp b/llvm/lib/Transforms/Utils/GuardUtils.cpp
index 7c310f16d46e2..6f0a665796590 100644
--- a/llvm/lib/Transforms/Utils/GuardUtils.cpp
+++ b/llvm/lib/Transforms/Utils/GuardUtils.cpp
@@ -11,6 +11,7 @@
#include "llvm/Transforms/Utils/GuardUtils.h"
#include "llvm/Analysis/GuardUtils.h"
+#include "llvm/Analysis/LoopInfo.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/Instructions.h"
@@ -78,8 +79,18 @@ void llvm::makeGuardControlFlowExplicit(Function *DeoptIntrinsic,
}
}
+#ifndef NDEBUG
+static bool isLoopInvariantWidenableCond(Value *WCCond, const Loop &L) {
+ bool IsLoopInv = L.isLoopInvariant(WCCond);
+ if (IsLoopInv || !isa<Instruction>(WCCond))
+ return IsLoopInv;
-void llvm::widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond) {
+ return L.hasLoopInvariantOperands(cast<Instruction>(WCCond));
+}
+#endif
+
+void llvm::widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond,
+ const LoopInfo &LI) {
assert(isWidenableBranch(WidenableBR) && "precondition");
// The tempting trivially option is to produce something like this:
@@ -90,6 +101,13 @@ void llvm::widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond) {
Use *C, *WC;
BasicBlock *IfTrueBB, *IfFalseBB;
parseWidenableBranch(WidenableBR, C, WC, IfTrueBB, IfFalseBB);
+#ifndef NDEBUG
+ bool IsLoopInvariantWidenableBR = false;
+ auto *L = LI.getLoopFor(WidenableBR->getParent());
+ if (L)
+ IsLoopInvariantWidenableBR =
+ isLoopInvariantWidenableCond(WidenableBR->getCondition(), *L);
+#endif
if (!C) {
// br (wc()), ... form
IRBuilder<> B(WidenableBR);
@@ -102,15 +120,33 @@ void llvm::widenWidenableBranch(BranchInst *WidenableBR, Value *NewCond) {
// Condition is only guaranteed to dominate branch
WCAnd->moveBefore(WidenableBR);
}
+#ifndef NDEBUG
+ if (IsLoopInvariantWidenableBR) {
+ auto *L = LI.getLoopFor(WidenableBR->getParent());
+ // NB! This can also be loop-varying if we missed hoisting the IR out, which
+ // was infact loop-invariant. This would imply a pass-ordering issue and the
+ // assert should be dealt with as such.
+ assert(isLoopInvariantWidenableCond(WidenableBR->getCondition(), *L) &&
+ "Loop invariant widenable condition made loop variant!");
+ }
+#endif
assert(isWidenableBranch(WidenableBR) && "preserve widenabiliy");
}
-void llvm::setWidenableBranchCond(BranchInst *WidenableBR, Value *NewCond) {
+void llvm::setWidenableBranchCond(BranchInst *WidenableBR, Value *NewCond,
+ const LoopInfo &LI) {
assert(isWidenableBranch(WidenableBR) && "precondition");
Use *C, *WC;
BasicBlock *IfTrueBB, *IfFalseBB;
parseWidenableBranch(WidenableBR, C, WC, IfTrueBB, IfFalseBB);
+#ifndef NDEBUG
+ bool IsLoopInvariantWidenableBR = false;
+ auto *L = LI.getLoopFor(WidenableBR->getParent());
+ if (L)
+ IsLoopInvariantWidenableBR =
+ isLoopInvariantWidenableCond(WidenableBR->getCondition(), *L);
+#endif
if (!C) {
// br (wc()), ... form
IRBuilder<> B(WidenableBR);
@@ -122,5 +158,15 @@ void llvm::setWidenableBranchCond(BranchInst *WidenableBR, Value *NewCond) {
WCAnd->moveBefore(WidenableBR);
C->set(NewCond);
}
+#ifndef NDEBUG
+ if (IsLoopInvariantWidenableBR) {
+ auto *L = LI.getLoopFor(WidenableBR->getParent());
+ // NB! This can also be loop-varying if we missed hoisting the IR out, which
+ // was infact loop-invariant. This would imply a pass-ordering issue and the
+ // assert should be dealt with as such.
+ assert(isLoopInvariantWidenableCond(WidenableBR->getCondition(), *L) &&
+ "Loop invariant widenable condition made loop variant!");
+ }
+#endif
assert(isWidenableBranch(WidenableBR) && "preserve widenabiliy");
}
More information about the llvm-commits
mailing list