[PATCH] D60396: [InstCombine] sdiv exact flag fixup
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 8 06:01:35 PDT 2019
spatel added inline comments.
================
Comment at: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1696
if (match(Op1, m_SDiv(m_Value(X), m_Constant(C))) && match(Op0, m_Zero()) &&
- C->isNotMinSignedValue() && !C->isOneValue())
- return BinaryOperator::CreateSDiv(X, ConstantExpr::getNeg(C));
+ C->isNotMinSignedValue() && !C->isOneValue()) {
+ auto *BO = BinaryOperator::CreateSDiv(X, ConstantExpr::getNeg(C));
----------------
lebedev.ri wrote:
> Uhm.
> So, this is not related to this patch specifically, but i think `isOneValue()` check is incorrect,
> and //may// be causing miscompiles: https://godbolt.org/z/CmAslh <- i don't think `@test_exact_vec` should be folded?
> I think you want to add a `isNotOneValue()`.
> Also, is this fold valid for `undef` elements?
>
> CC @spatel
Yes, if this patch has not been reverted yet, it probably should be.
It's not correct for arbitrary vector constants. We can either limit this to splat constants (m_APInt) or do the more involved element-by-element check.
If the divisor has an undef element, we have immediate UB, so anything goes. But we might want to do something better? I haven't looked at this patch closely yet.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60396/new/
https://reviews.llvm.org/D60396
More information about the llvm-commits
mailing list