[PATCH] D52504: [DAGCombiner] Div/rem folds

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:3103
-  // TODO: X % 1 -> 0
-  // If this is a boolean op (single-bit element type), we can't have
-  // division-by-zero or remainder-by-zero, so assume the divisor is 1.
----------------
xbolva00 wrote:
> spatel wrote:
> > 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
> > }
> > 
> > ```
> 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?
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.)


https://reviews.llvm.org/D52504