From 5528388e3664c6d7d292f20a739f1bf1c8ef768d Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Fri, 13 Dec 2024 22:06:39 +0000 Subject: [PATCH] EarlyCSE: fix CmpPredicate duplicate-hashing (#119902) Strip hash_value() for CmpPredicate, as different callers have different hashing use-cases. In this case, there is just one caller, namely EarlyCSE, which calls hash_combine() on a CmpPredicate, which used to call hash_combine() on a CmpInst::Predicate prior to 4a0d53a (PatternMatch: migrate to CmpPredicate). This has uncovered a bug where two icmp instructions differing in just the fact that one of them has the samesign flag on it are hashed differently, leading to divergent hashing, and a crash. Fix this crash by dropping samesign information on icmp instructions before hashing them, preserving the former behavior. Fixes #119893. --- llvm/include/llvm/IR/CmpPredicate.h | 6 ------ llvm/lib/IR/Instructions.cpp | 4 ---- llvm/lib/Transforms/Scalar/EarlyCSE.cpp | 3 ++- llvm/test/Transforms/EarlyCSE/pr119893.ll | 21 +++++++++++++++++++++ 4 files changed, 23 insertions(+), 11 deletions(-) create mode 100644 llvm/test/Transforms/EarlyCSE/pr119893.ll diff --git a/llvm/include/llvm/IR/CmpPredicate.h b/llvm/include/llvm/IR/CmpPredicate.h index ce78e4311f9f82..9aa1449465f5ff 100644 --- a/llvm/include/llvm/IR/CmpPredicate.h +++ b/llvm/include/llvm/IR/CmpPredicate.h @@ -71,13 +71,7 @@ class CmpPredicate { /// Get the swapped predicate of a CmpInst. static CmpPredicate getSwapped(const CmpInst *Cmp); - - /// Provided to facilitate storing a CmpPredicate in data structures that - /// require hashing. - friend hash_code hash_value(const CmpPredicate &Arg); // NOLINT }; - -[[nodiscard]] hash_code hash_value(const CmpPredicate &Arg); } // namespace llvm #endif diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp index d1da02c744f18c..2d6fe40f4c1de0 100644 --- a/llvm/lib/IR/Instructions.cpp +++ b/llvm/lib/IR/Instructions.cpp @@ -3946,10 +3946,6 @@ CmpPredicate CmpPredicate::getSwapped(const CmpInst *Cmp) { return getSwapped(get(Cmp)); } -hash_code llvm::hash_value(const CmpPredicate &Arg) { // NOLINT - return hash_combine(Arg.Pred, Arg.HasSameSign); -} - //===----------------------------------------------------------------------===// // SwitchInst Implementation //===----------------------------------------------------------------------===// diff --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp index 682c5c3d8c6340..3a0ae6b01a1144 100644 --- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp +++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp @@ -290,7 +290,8 @@ static unsigned getHashValueImpl(SimpleValue Val) { Pred = CmpInst::getInversePredicate(Pred); std::swap(A, B); } - return hash_combine(Inst->getOpcode(), Pred, X, Y, A, B); + return hash_combine(Inst->getOpcode(), + static_cast(Pred), X, Y, A, B); } if (CastInst *CI = dyn_cast(Inst)) diff --git a/llvm/test/Transforms/EarlyCSE/pr119893.ll b/llvm/test/Transforms/EarlyCSE/pr119893.ll new file mode 100644 index 00000000000000..c9ea315c9fea47 --- /dev/null +++ b/llvm/test/Transforms/EarlyCSE/pr119893.ll @@ -0,0 +1,21 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 +; RUN: opt -passes=early-cse -S %s | FileCheck %s + +define i32 @samesign_hash_bug(i16 %v) { +; CHECK-LABEL: define i32 @samesign_hash_bug( +; CHECK-SAME: i16 [[V:%.*]]) { +; CHECK-NEXT: [[ZEXT:%.*]] = zext i16 [[V]] to i32 +; CHECK-NEXT: [[ICMP_SAMESIGN:%.*]] = icmp ugt i32 [[ZEXT]], 31 +; CHECK-NEXT: [[SELECT_ICMP_SAMESIGN:%.*]] = select i1 [[ICMP_SAMESIGN]], i32 0, i32 1 +; CHECK-NEXT: [[SELECT_ICMP:%.*]] = select i1 [[ICMP_SAMESIGN]], i32 1, i32 0 +; CHECK-NEXT: ret i32 [[SELECT_ICMP]] +; + %zext = zext i16 %v to i32 + %icmp.samesign = icmp samesign ugt i32 %zext, 31 + %select.icmp.samesign = select i1 %icmp.samesign, i32 0, i32 1 + %ashr = ashr i32 0, %select.icmp.samesign + %icmp = icmp ugt i32 %zext, 31 + %select.icmp = select i1 %icmp, i32 1, i32 0 + %ret = add i32 %ashr, %select.icmp + ret i32 %ret +}