Skip to content

Commit

Permalink
[ConstantFold] Avoid creation of undesirable binop
Browse files Browse the repository at this point in the history 
When commuting the operands, don't create a constant expression
for undesirable binops. Only invoke the constant folding function
in that case.
  • Loading branch information
@nikic
nikic committed Jul 25, 2023
1 parent b6cf0ea commit 673a467
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 3 deletions.
4 changes: 3 additions & 1 deletion llvm/lib/IR/ConstantFold.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,9 @@ Constant *llvm::ConstantFoldBinaryInstruction(unsigned Opcode, Constant *C1,
} else if (isa<ConstantInt>(C1)) {
// If C1 is a ConstantInt and C2 is not, swap the operands.
if (Instruction::isCommutative(Opcode))
return ConstantExpr::get(Opcode, C2, C1);
return ConstantExpr::isDesirableBinOp(Opcode)
? ConstantExpr::get(Opcode, C2, C1)
: ConstantFoldBinaryInstruction(Opcode, C2, C1);
}

if (ConstantInt *CI1 = dyn_cast<ConstantInt>(C1)) {
Expand Down
5 changes: 3 additions & 2 deletions llvm/test/Transforms/InstCombine/pr32686.ll
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ define void @tinkywinky() {
; CHECK-LABEL: @tinkywinky(
; CHECK-NEXT: [[PATATINO:%.*]] = load i8, ptr @a, align 1
; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i8 [[PATATINO]], 0
; CHECK-NEXT: [[TMP1:%.*]] = zext i1 [[TOBOOL_NOT]] to i32
; CHECK-NEXT: [[OR1:%.*]] = or i32 [[TMP1]], or (i32 zext (i1 icmp ne (ptr @a, ptr @b) to i32), i32 2)
; CHECK-NEXT: [[TMP1:%.*]] = or i1 [[TOBOOL_NOT]], icmp ne (ptr @a, ptr @b)
; CHECK-NEXT: [[TMP2:%.*]] = zext i1 [[TMP1]] to i32
; CHECK-NEXT: [[OR1:%.*]] = or i32 [[TMP2]], 2
; CHECK-NEXT: store i32 [[OR1]], ptr @b, align 4
; CHECK-NEXT: ret void
;
Expand Down

4 comments on commit 673a467

@nunoplopes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing a lot of crashes after this commit when compiling LLVM with gcc 13.2 (latest fedora):

$ opt -S -p='require<opt-remark-emit>,loop-unroll<peeling;no-runtime>' -unroll-runtime=true -unroll-runtime-epilog=true ~/llvm/llvm/test/Transforms/LoopUnroll/runtime-loop.ll
opt: llvm/llvm/include/llvm/Support/Casting.h:578: decltype(auto) llvm::cast(From*) [with To = ConstantInt; From = Constant]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
...
 #9 llvm::ScalarEvolution::getNegativeSCEV(llvm::SCEV const*, llvm::SCEV::NoWrapFlags)
#10 llvm::ScalarEvolution::getMinusSCEV(llvm::SCEV const*, llvm::SCEV const*, llvm::SCEV::NoWrapFlags, unsigned int)
#11 llvm::ScalarEvolution::isKnownPredicateViaConstantRanges(llvm::CmpInst::Predicate, llvm::SCEV const*, llvm::SCEV const*)
#12 llvm::ScalarEvolution::isKnownViaNonRecursiveReasoning(llvm::CmpInst::Predicate, llvm::SCEV const*, llvm::SCEV const*)
#13 llvm::ScalarEvolution::isLoopEntryGuardedByCond(llvm::Loop const*, llvm::CmpInst::Predicate, llvm::SCEV const*, llvm::SCEV const*)
#14 llvm::ScalarEvolution::howFarToZero(llvm::SCEV const*, llvm::Loop const*, bool, bool)
#15 llvm::ScalarEvolution::computeExitLimitFromICmp(llvm::Loop const*, llvm::CmpInst::Predicate, llvm::SCEV const*, llvm::SCEV const*, bool, bool)
#16 llvm::ScalarEvolution::computeExitLimitFromICmp(llvm::Loop const*, llvm::ICmpInst*, bool, bool, bool)
#17 llvm::ScalarEvolution::computeExitLimitFromCondImpl(llvm::ScalarEvolution::ExitLimitCache&, llvm::Loop const*, llvm::Value*, bool, bool, bool)
#18 llvm::ScalarEvolution::computeExitLimitFromCondCached(llvm::ScalarEvolution::ExitLimitCache&, llvm::Loop const*, llvm::Value*, bool, bool, bool)
#19 llvm::ScalarEvolution::computeExitLimitFromCond(llvm::Loop const*, llvm::Value*, bool, bool, bool)
#20 llvm::ScalarEvolution::computeExitLimit(llvm::Loop const*, llvm::BasicBlock*, bool)
#21 llvm::ScalarEvolution::computeBackedgeTakenCount(llvm::Loop const*, bool)
#22 llvm::ScalarEvolution::getBackedgeTakenInfo(llvm::Loop const*)
#23 llvm::ScalarEvolution::getSmallConstantTripCount(llvm::Loop const*, llvm::BasicBlock const*)
#24 tryToUnrollLoop(...)
#25 llvm::LoopUnrollPass::run(llvm::Function&, llvm::AnalysisManager<llvm::Function>&)

I've more than 1k failures in the test suite with similar crashes. I've tried to bisect a few of them, but they all end up in this commit.
Does this ring any bell?

@nikic
Copy link
Contributor Author

@nikic nikic commented on 673a467 Dec 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I haven't seen this kind of issue. What's your cmake config?

@nunoplopes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmake -GNinja -DLLVM_ENABLE_RTTI=ON -DLLVM_ENABLE_EH=ON -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS="llvm;clang" ../llvm

@nunoplopes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried a more normal build (cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS="llvm;clang" ../llvm) and I get the same crash.
I'm starting to wonder if it isn't a bug in gcc 13..

Please sign in to comment.