Skip to content

Commit

Permalink
EarlyCSE: fix CmpPredicate duplicate-hashing (#119902)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
artagnon authored Dec 13, 2024
1 parent cd093c2 commit 5528388
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 11 deletions.
6 changes: 0 additions & 6 deletions llvm/include/llvm/IR/CmpPredicate.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 0 additions & 4 deletions llvm/lib/IR/Instructions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
//===----------------------------------------------------------------------===//
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Transforms/Scalar/EarlyCSE.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<CmpInst::Predicate>(Pred), X, Y, A, B);
}

if (CastInst *CI = dyn_cast<CastInst>(Inst))
Expand Down
21 changes: 21 additions & 0 deletions llvm/test/Transforms/EarlyCSE/pr119893.ll
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit 5528388

Please sign in to comment.