Fold add nuw and uadd.with.overflow with constants if the
addition does not overflow.
Details
Diff Detail
Event Timeline
Looks reasonable, test coverage nits added.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
---|---|---|
2078–2081 | Are these tested? |
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp | ||
---|---|---|
2078–2081 | foldIntrinsicWithOverflowCommon is a very thin wrapper around OptimizeOverflowCheck which is tested in with_overflow.ll. I couldn't find any tests in which canonicalizeConstantArg0ToArg1 would be used. I can add a test for it in with_overflow.ll. |
Logic looks fine, so I won't hold it up, but seems better to not duplicate code for sibling transforms?
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp index 49524d7f42e..9de4e60c1b9 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp @@ -2046,6 +2046,7 @@ Instruction *InstCombiner::visitCallInst(CallInst &CI) { return &CI; break; } + case Intrinsic::uadd_with_overflow: case Intrinsic::sadd_with_overflow: { if (Instruction *I = canonicalizeConstantArg0ToArg1(CI)) return I; @@ -2053,25 +2054,28 @@ Instruction *InstCombiner::visitCallInst(CallInst &CI) { return I; // Given 2 constant operands whose sum does not overflow: + // uaddo (X +nuw C0), C1 -> uaddo X, C0 + C1 // saddo (X +nsw C0), C1 -> saddo X, C0 + C1 + bool IsUnsigned = II->getIntrinsicID() == Intrinsic::uadd_with_overflow; Value *X; const APInt *C0, *C1; Value *Arg0 = II->getArgOperand(0); Value *Arg1 = II->getArgOperand(1); - if (match(Arg0, m_NSWAdd(m_Value(X), m_APInt(C0))) && - match(Arg1, m_APInt(C1))) { + bool HasAdd = IsUnsigned ? match(Arg0, m_NUWAdd(m_Value(X), m_APInt(C0))) + : match(Arg0, m_NSWAdd(m_Value(X), m_APInt(C0))); + if (HasAdd && match(Arg1, m_APInt(C1))) { bool Overflow; - APInt NewC = C1->sadd_ov(*C0, Overflow); + APInt NewC = IsUnsigned ? C1->uadd_ov(*C0, Overflow) + : C1->sadd_ov(*C0, Overflow); if (!Overflow) return replaceInstUsesWith( *II, Builder.CreateBinaryIntrinsic( - Intrinsic::sadd_with_overflow, X, + II->getIntrinsicID(), X, ConstantInt::get(Arg1->getType(), NewC))); } break; } - case Intrinsic::uadd_with_overflow: case Intrinsic::umul_with_overflow: case Intrinsic::smul_with_overflow: if (Instruction *I = canonicalizeConstantArg0ToArg1(CI))
I agree, the duplication doesn't look nice.
It may indeed be better to condense the cases,
and have a standalone switch for more specific opts
that will come up in the future,
Your change looks much better. I wanted to do something like this initially, but I couldn't figure it out. I was missing getIntrinsicId. I'll update this shortly.
Are these tested?