-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
[CGP] Drop poison-generating flags after hoisting #90382
Conversation
@llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesSee the following case:
Proof: https://alive2.llvm.org/ce/z/sP7mvK Full diff: https://github.com/llvm/llvm-project/pull/90382.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 8eaf78157550ee..9e9c0a09190f37 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -8268,8 +8268,10 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
if (CmpC.isPowerOf2() && Cmp->getPredicate() == ICmpInst::ICMP_ULT &&
match(UI, m_Shr(m_Specific(X), m_SpecificInt(CmpC.logBase2())))) {
IRBuilder<> Builder(Branch);
- if (UI->getParent() != Branch->getParent())
+ if (UI->getParent() != Branch->getParent()) {
UI->moveBefore(Branch);
+ UI->dropPoisonGeneratingFlags();
+ }
Value *NewCmp = Builder.CreateCmp(ICmpInst::ICMP_EQ, UI,
ConstantInt::get(UI->getType(), 0));
LLVM_DEBUG(dbgs() << "Converting " << *Cmp << "\n");
@@ -8281,8 +8283,10 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
(match(UI, m_Add(m_Specific(X), m_SpecificInt(-CmpC))) ||
match(UI, m_Sub(m_Specific(X), m_SpecificInt(CmpC))))) {
IRBuilder<> Builder(Branch);
- if (UI->getParent() != Branch->getParent())
+ if (UI->getParent() != Branch->getParent()) {
UI->moveBefore(Branch);
+ UI->dropPoisonGeneratingFlags();
+ }
Value *NewCmp = Builder.CreateCmp(Cmp->getPredicate(), UI,
ConstantInt::get(UI->getType(), 0));
LLVM_DEBUG(dbgs() << "Converting " << *Cmp << "\n");
diff --git a/llvm/test/Transforms/CodeGenPrepare/RISCV/convert-to-eqz.ll b/llvm/test/Transforms/CodeGenPrepare/RISCV/convert-to-eqz.ll
new file mode 100644
index 00000000000000..a08ca34f01a9f5
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/RISCV/convert-to-eqz.ll
@@ -0,0 +1,54 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -codegenprepare -S -mtriple=riscv64 < %s | FileCheck %s
+
+define i8 @hoist_add(i8 %x) {
+; CHECK-LABEL: define i8 @hoist_add(
+; CHECK-SAME: i8 [[X:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[INC:%.*]] = add i8 [[X]], 1
+; CHECK-NEXT: [[TMP0:%.*]] = icmp eq i8 [[INC]], 0
+; CHECK-NEXT: br i1 [[TMP0]], label [[EXIT:%.*]], label [[IF_THEN:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: br label [[EXIT]]
+; CHECK: exit:
+; CHECK-NEXT: [[RETVAL:%.*]] = phi i8 [ [[INC]], [[IF_THEN]] ], [ -1, [[ENTRY:%.*]] ]
+; CHECK-NEXT: ret i8 [[RETVAL]]
+;
+entry:
+ %cmp = icmp eq i8 %x, -1
+ br i1 %cmp, label %exit, label %if.then
+
+if.then:
+ %inc = add nuw nsw i8 %x, 1
+ br label %exit
+
+exit:
+ %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ]
+ ret i8 %retval
+}
+
+define i8 @hoist_lshr(i8 %x) {
+; CHECK-LABEL: define i8 @hoist_lshr(
+; CHECK-SAME: i8 [[X:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[INC:%.*]] = lshr i8 [[X]], 3
+; CHECK-NEXT: [[TMP0:%.*]] = icmp eq i8 [[INC]], 0
+; CHECK-NEXT: br i1 [[TMP0]], label [[EXIT:%.*]], label [[IF_THEN:%.*]]
+; CHECK: if.then:
+; CHECK-NEXT: br label [[EXIT]]
+; CHECK: exit:
+; CHECK-NEXT: [[RETVAL:%.*]] = phi i8 [ [[INC]], [[IF_THEN]] ], [ -1, [[ENTRY:%.*]] ]
+; CHECK-NEXT: ret i8 [[RETVAL]]
+;
+entry:
+ %cmp = icmp ult i8 %x, 8
+ br i1 %cmp, label %exit, label %if.then
+
+if.then:
+ %inc = lshr exact i8 %x, 3
+ br label %exit
+
+exit:
+ %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ]
+ ret i8 %retval
+}
|
I just wonder if there are possibly other places in CGP where we should drop the flags, but I assume that, unless we have a test case showing that explicitly, it's fine as it is. |
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, but is it possible to add these tests in llvm/test/Transforms/CodeGenPrepare/ARM/branch-on-zero.ll, which has the other tests for this transform?
/cherry-pick ab12bba |
See the following case: ``` define i8 @SRC1(i8 %x) { entry: %cmp = icmp eq i8 %x, -1 br i1 %cmp, label %exit, label %if.then if.then: %inc = add nuw nsw i8 %x, 1 br label %exit exit: %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ] ret i8 %retval } define i8 @tgt1(i8 %x) { entry: %inc = add nuw nsw i8 %x, 1 %0 = icmp eq i8 %inc, 0 br i1 %0, label %exit, label %if.then if.then: ; preds = %entry br label %exit exit: ; preds = %if.then, %entry %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ] ret i8 %retval } ``` `optimizeBranch` converts `icmp eq X, -1` into cmp to zero on RISC-V and hoists the add into the entry block. Poison-generating flags should be dropped as they don't still hold. Proof: https://alive2.llvm.org/ce/z/sP7mvK Fixes llvm#90380 (cherry picked from commit ab12bba)
/pull-request #90437 |
See the following case: ``` define i8 @SRC1(i8 %x) { entry: %cmp = icmp eq i8 %x, -1 br i1 %cmp, label %exit, label %if.then if.then: %inc = add nuw nsw i8 %x, 1 br label %exit exit: %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ] ret i8 %retval } define i8 @tgt1(i8 %x) { entry: %inc = add nuw nsw i8 %x, 1 %0 = icmp eq i8 %inc, 0 br i1 %0, label %exit, label %if.then if.then: ; preds = %entry br label %exit exit: ; preds = %if.then, %entry %retval = phi i8 [ %inc, %if.then ], [ -1, %entry ] ret i8 %retval } ``` `optimizeBranch` converts `icmp eq X, -1` into cmp to zero on RISC-V and hoists the add into the entry block. Poison-generating flags should be dropped as they don't still hold. Proof: https://alive2.llvm.org/ce/z/sP7mvK Fixes llvm#90380 (cherry picked from commit ab12bba)
See the following case:
optimizeBranch
convertsicmp eq X, -1
into cmp to zero on RISC-V and hoists the add into the entry block. Poison-generating flags should be dropped as they don't still hold.Proof: https://alive2.llvm.org/ce/z/sP7mvK
Fixes #90380