This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] sdiv exact flag fixup
ClosedPublic

Authored by shchenz on Apr 7 2019, 11:00 PM.

Details

Summary

This is a issue found when investigate https://reviews.llvm.org/D60395.
exact flag is missing when opt combine 0 - (X sdiv C) -> (X sdiv -C)

Diff Detail

Event Timeline

shchenz created this revision.Apr 7 2019, 11:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2019, 11:00 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Looks good, but a question re tests.

----------------------------------------
Optimization: zz
Precondition: ((C != 1) && (C != -128))
  %div = sdiv exact i8 %x, C
  %r = sub i8 0, %div
=>
  %r = sdiv exact i8 %x, -C

Done: 1
Optimization is correct!
llvm/test/Transforms/InstCombine/div.ll
763

Please add more tests.

  • Variant with nsw, but without exact
  • Variant without nsw, but with exact
  • Variant without nsw, and without exact
  • ???
  • Are there tests with C = 1, C = MinSigned ?
shchenz updated this revision to Diff 194101.Apr 8 2019, 1:25 AM

address comments

please precommit the tests

llvm/test/Transforms/InstCombine/div.ll
783

tests are faulty, contain nsw despite the name

shchenz updated this revision to Diff 194107.Apr 8 2019, 2:25 AM

address Roman's comments.

shchenz marked an inline comment as done.Apr 8 2019, 2:27 AM

Thanks for your comments Roman @lebedev.ri . Updated.

llvm/test/Transforms/InstCombine/div.ll
783

oops. fixed.

lebedev.ri accepted this revision.Apr 8 2019, 3:34 AM

LG

llvm/test/Transforms/InstCombine/div.ll
781

test name is @test_exact_nonsw_noexact

This revision is now accepted and ready to land.Apr 8 2019, 3:34 AM
This revision was automatically updated to reflect the committed changes.
shchenz marked an inline comment as done.
lebedev.ri added inline comments.Apr 8 2019, 5:22 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

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

spatel added inline comments.Apr 8 2019, 6:00 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

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.

lebedev.ri added inline comments.Apr 8 2019, 6:24 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

Note that *this* patch only fixed propagation of exact.
I'm wondering if the entire fold is broken for vectors w/undef elts.

lebedev.ri added inline comments.Apr 8 2019, 6:26 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

... and it won't be possible to revert the original patch, since it is here for 9+ years.
So i think this fold should be fixed instead.

nikic added a subscriber: nikic.Apr 8 2019, 6:33 AM
spatel added inline comments.Apr 8 2019, 6:34 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

Ah, sorry - didn't see it clearly the 1st time. Yes, I agree the bug was already there, so we just need to fix it as a follow-up.

Do you or @shchenz want to write that patch?

lebedev.ri added inline comments.Apr 8 2019, 6:36 AM
llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)
shchenz marked an inline comment as done.Apr 8 2019, 6:42 AM
shchenz added inline comments.
llvm/test/Transforms/InstCombine/div.ll
781

fixed.

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp
1696 ↗(On Diff #194116)

thanks for pointing out this bug. I am afraid I don't have time for this issue now.