This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombiner] Fold 0 div/rem X to 0
ClosedPublic

Authored by xbolva00 on Sep 25 2018, 9:07 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Sep 25 2018, 9:07 AM
xbolva00 updated this revision to Diff 166934.Sep 25 2018, 9:13 AM
RKSimon added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3146–3149

You're ignoring this comment - consult simplifyDivRem inside InstructionSimplify.cpp which shows the issue

IIRC last time I started looking at this a load of tests failed in other targets, including some reduced test cases that needed fixing.

xbolva00 added inline comments.Sep 25 2018, 9:46 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3146–3149

Seems like we don't even need the X /, % 1 -> X fold here. With or without - tests are not affected, this was folded even before this patch.

xbolva00 updated this revision to Diff 166962.Sep 25 2018, 11:25 AM

Updated tests

RKSimon added inline comments.Sep 26 2018, 2:31 AM
test/CodeGen/X86/pr38539.ll
6 ↗(On Diff #166962)

These test cases need rebuilding so that they still check for PR38539

test/CodeGen/X86/shrink_vmul.ll
2233 ↗(On Diff #166962)

This test needs updating to still check for this bug - I'll try to fix this one

xbolva00 added inline comments.Sep 26 2018, 5:02 AM
test/CodeGen/X86/pr38539.ll
6 ↗(On Diff #166962)

So change %B8 = srem i66 0, %B20 to e.g.
%B8 = srem i66 3 (any value which does not fold), %B20 should be okay?

RKSimon added inline comments.
test/CodeGen/X86/pr38539.ll
6 ↗(On Diff #166962)

@craig.topper will be able to confirm but it might be that you need to (locally) revert the fix for PR38539, apply your DAGCombiner.cpp patch and then run bugpoint to reduce the original fuzz code again: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=8882

spatel added inline comments.Sep 26 2018, 8:44 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3141

Please don't remove this comment without actually fixing the underlying problem (in a separate patch).
I think it would be better to consolidate the existing code here for 2 reasons:

  1. It's cleaner to have all of the related folds in 1 place.
  2. The existing simplification for urem/srem-by-1 is not particularly efficient - it converts to an 'and' first and then simplifies the 'and'.

Here are test cases you can add/use to confirm the difference between llc and opt for the boolean patterns:

define i1 @bool_udiv(i1 %x, i1 %y) {
  %r = udiv i1 %x, %y
  ret i1 %r
}

define i1 @bool_sdiv(i1 %x, i1 %y) {
  %r = sdiv i1 %x, %y
  ret i1 %r
}

define i1 @bool_urem(i1 %x, i1 %y) {
  %r = urem i1 %x, %y
  ret i1 %r
}

define i1 @bool_srem(i1 %x, i1 %y) {
  %r = srem i1 %x, %y
  ret i1 %r
}

define <4 x i1> @boolvec_udiv(<4 x i1> %x, <4 x i1> %y) {
  %r = udiv <4 x i1> %x, %y
  ret <4 x i1> %r
}

define <4 x i1> @boolvec_sdiv(<4 x i1> %x, <4 x i1> %y) {
  %r = sdiv <4 x i1> %x, %y
  ret <4 x i1> %r
}

define <4 x i1> @boolvec_urem(<4 x i1> %x, <4 x i1> %y) {
  %r = urem <4 x i1> %x, %y
  ret <4 x i1> %r
}

define <4 x i1> @boolvec_srem(<4 x i1> %x, <4 x i1> %y) {
  %r = srem <4 x i1> %x, %y
  ret <4 x i1> %r
}
xbolva00 added inline comments.Sep 26 2018, 10:42 AM
test/CodeGen/X86/pr38539.ll
6 ↗(On Diff #166962)

I reverted https://reviews.llvm.org/rL339945 and I ran llvm-lit on pr38539.ll - test passed... (flags-copy-lowering.mir failed). Something else between that commit and top of trunk must fix it too.

So I think we should either remove test file or modify it with no worries.

xbolva00 added inline comments.Sep 26 2018, 10:49 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3141

I will revert that comment removal..

Anyway, I see

// fold (sdiv X, 1) -> X

if (N1C && N1C->isOne())
  return N0;

in visitS(U)DIV so.. do you suggest to unify it and move it here?

spatel added inline comments.Sep 26 2018, 10:56 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
3141

Yes, I think that's a good change - reduce code duplication and more efficient (for urem/srem). You can do that cleanup first as an NFC patch. Then, add the functionality for bool types as a follow-up. (Both changes are independent of this patch.)

As with the earlier patches, we need to try to keep the tests that are checking for bugs or other things relevant. I tried to do that for the x86 tests here:
rL345639
rL345640
rL345642

Please do something like that for the ARM tests, so they are not affected by this patch.

xbolva00 updated this revision to Diff 171822.Oct 30 2018, 3:18 PM
  • Rebased

Sanjay's changes don't seem to be rebased ?

xbolva00 updated this revision to Diff 171907.Oct 31 2018, 6:44 AM

Sanjay's changes don't seem to be rebased ?

Done

RKSimon accepted this revision.Oct 31 2018, 7:03 AM

LGTM

This revision is now accepted and ready to land.Oct 31 2018, 7:03 AM

Please can you make the subject more descriptive before you commit

xbolva00 retitled this revision from [DAGCombiner] Div/rem folds to [DAGCombiner] Fold 0 div/rem X to 0.Oct 31 2018, 7:08 AM
This revision was automatically updated to reflect the committed changes.