[DAGCombiner] Add X / X -> 1 & X % X -> 0 folds.

[RKSimon]

Summary

Adds more divrem folds to try and get in sync with InstructionSimplify

This hits a number of reduced tests so I've had to tweak them; through a mixture of visual scrutiny and debugging they seem to be hitting the same points as the original test failed at.

[jonpa]

Not sure if I'm doing something wrong (I use git and tried first patch -p0, and then also with dos2unix, but then that didn't work so I had to write the patch manually although just a few lines...)

This did not give any change at all on benchmarks on SystemZ (NFC), and the SystemZ test succeeds with or without the patch...?

[RKSimon]

In D50636#1197251, @jonpa wrote:

Not sure if I'm doing something wrong (I use git and tried first patch -p0, and then also with dos2unix, but then that didn't work so I had to write the patch manually although just a few lines...)

Sorry - I've never worked out why the upload always screws up my line endings.....

This did not give any change at all on benchmarks on SystemZ (NFC), and the SystemZ test succeeds with or without the patch...?

Are the x86 tests changing? They contain explicit tests for this fold

[jonpa]

In D50636#1197255, @RKSimon wrote:

In D50636#1197251, @jonpa wrote:

Not sure if I'm doing something wrong (I use git and tried first patch -p0, and then also with dos2unix, but then that didn't work so I had to write the patch manually although just a few lines...)

Sorry - I've never worked out why the upload always screws up my line endings.....

This did not give any change at all on benchmarks on SystemZ (NFC), and the SystemZ test succeeds with or without the patch...?

Are the x86 tests changing? They contain explicit tests for this fold

Yes, change in x86 test (at least the one I tried)...

[RKSimon]

test/CodeGen/SystemZ/pr32372.ll

18 ↗

(On Diff #160352)

Tried again against bleeding edge trunk and I still see this change and without the volatile (which prevents the loads being combined) the checks reduce to:

define void @pr32372(i8*) {
; CHECK-LABEL: pr32372:
; CHECK:       # %bb.0: # %BB
; CHECK-NEXT:    mvhhi 0(%r1), -3825
; CHECK-NEXT:  .LBB0_1: # %CF251
; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
; CHECK-NEXT:    j .LBB0_1

[craig.topper]

test/CodeGen/X86/combine-srem.ll

455 ↗

(On Diff #160352)

Should these test IR changes be committed separately?

[jonpa]

test/CodeGen/SystemZ/pr32372.ll

18 ↗

(On Diff #160352)

Yes, I see the change after updating the test, but also note that this updated test passes also without the patch... So I am not sure why the test is changed if the patch does not affect it...

[craig.topper]

test/CodeGen/SystemZ/pr32372.ll

18 ↗

(On Diff #160352)

I think this patch affects the original version of the test. Without the volatile the two loads get CSEd to the same SDNode. This causes the same SDNode to be both operands of the urem. This patch would simplify that to 0. Simon added the volatile to prevent the loads from being CSEd. This way the urem won't get affected by this patch. So the new version should pass on trunk as well.

[jonpa]

test/CodeGen/SystemZ/pr32372.ll

18 ↗

(On Diff #160352)

Ah, I see. Thank you for the explanation.

[RKSimon]

I'm happy to apply the test changes beforehand once everybody is happy with the patch - but I'll keep them in the diff for now to make its effects clear.

test/CodeGen/SystemZ/pr32372.ll

18 ↗

(On Diff #160352)

Sorry - I missed that you were applying the test changes before the code changes!

[craig.topper]

The DAGCombiner changes and the X86 test changes all look good to me.

[RKSimon]

@jonpa @pftbest Any more comments?

[jonpa]

The origin for that test case I believe is that I had found a crash during testing and I reported this as PR32372.

The test changes here look OK to me, but when I reverted the fix for that original bug it seems the new test does no longer fail, while the unmodified one still does... :-/

Unfortunately, I don't seem to have the unreduced test failure available, so I can't make a new reduced version or even try if the original test case still fails...

Not sure if we should remove this test then, or keep it in case it may trigger somehow in the future...? Of course, the assert that the test used to trigger is still there, so removing it isn't all that unthinkable to me...

I tried some other experiments to the test, such as having two i8* arguments - one for each load - but that did not help...

[RKSimon]

Rebased patch

[craig.topper]

@jonpa, it looks like changing the urem to this will trigger the original bug and avoid the optimization Simon is adding here.

%B8 = call { i8, i1 } @llvm.umul.with.overflow.i8(i8 %L5, i8 %L)
%B9 = extractvalue {i8, i1} %B8, 0

[jonpa]

it looks like changing the urem to this will trigger the original bug and avoid the optimization Simon is adding here.

Ah, nice :-) I tried it, and just like you found that it triggers the original bug. I updated the test, patch is:

pr32372.ll.patch1 KBDownload

[RKSimon]

Applied @jonpa's test fix.

@craig.topper I also made a fix to vector-idiv-v2i32.ll which I think was a typo?

[craig.topper]

LGTM

llvm/trunk/test/CodeGen/MSP430/libcalls.ll

 }
 
 define i16 @divi() #0 {
 entry:
 ; CHECK: divi:
 
 ; CHECK: call #__mspabi_divi
   %0 = load volatile i16, i16* @g_i16, align 8
-  %1 = sdiv i16 %0, %0
+  %1 = load volatile i16, i16* @g_i16, align 8
+  %2 = sdiv i16 %0, %1
 
-  ret i16 %1
+  ret i16 %2
 }
 
 define i32 @divli() #0 {
 entry:
 ; CHECK: divli:
 
 ; CHECK: call #__mspabi_divli
   %0 = load volatile i32, i32* @g_i32, align 8
-  %1 = sdiv i32 %0, %0
+  %1 = load volatile i32, i32* @g_i32, align 8
+  %2 = sdiv i32 %0, %1
 
-  ret i32 %1
+  ret i32 %2
 }
 
 define i64 @divlli() #0 {
 entry:
 ; CHECK: divlli:
 
 ; CHECK: call #__mspabi_divlli
   %0 = load volatile i64, i64* @g_i64, align 8
-  %1 = sdiv i64 %0, %0
+  %1 = load volatile i64, i64* @g_i64, align 8
+  %2 = sdiv i64 %0, %1
 
-  ret i64 %1
+  ret i64 %2
 }
 
 define i16 @divu() #0 {
 entry:
 ; CHECK: divu:
 
 ; CHECK: call #__mspabi_divu
   %0 = load volatile i16, i16* @g_i16, align 8
-  %1 = udiv i16 %0, %0
+  %1 = load volatile i16, i16* @g_i16, align 8
+  %2 = udiv i16 %0, %1
 
-  ret i16 %1
+  ret i16 %2
 }
 
 define i32 @divul() #0 {
 entry:
 ; CHECK: divul:
 
 ; CHECK: call #__mspabi_divul
   %0 = load volatile i32, i32* @g_i32, align 8
-  %1 = udiv i32 %0, %0
+  %1 = load volatile i32, i32* @g_i32, align 8
+  %2 = udiv i32 %0, %1
 
-  ret i32 %1
+  ret i32 %2
 }
 
 define i64 @divull() #0 {
 entry:
 ; CHECK: divull:
 
 ; CHECK: call #__mspabi_divull
   %0 = load volatile i64, i64* @g_i64, align 8
-  %1 = udiv i64 %0, %0
+  %1 = load volatile i64, i64* @g_i64, align 8
+  %2 = udiv i64 %0, %1
 
-  ret i64 %1
+  ret i64 %2
 }
 
 define i16 @remi() #0 {
 entry:
 ; CHECK: remi:
 
 ; CHECK: call #__mspabi_remi
   %0 = load volatile i16, i16* @g_i16, align 8
-  %1 = srem i16 %0, %0
+  %1 = load volatile i16, i16* @g_i16, align 8
+  %2 = srem i16 %0, %1
 
-  ret i16 %1
+  ret i16 %2
 }
 
 define i32 @remli() #0 {
 entry:
 ; CHECK: remli:
 
 ; CHECK: call #__mspabi_remli
   %0 = load volatile i32, i32* @g_i32, align 8
-  %1 = srem i32 %0, %0
+  %1 = load volatile i32, i32* @g_i32, align 8
+  %2 = srem i32 %0, %1
 
-  ret i32 %1
+  ret i32 %2
 }
 
 define i64 @remlli() #0 {
 entry:
 ; CHECK: remlli:
 
 ; CHECK: call #__mspabi_remlli
   %0 = load volatile i64, i64* @g_i64, align 8
-  %1 = srem i64 %0, %0
+  %1 = load volatile i64, i64* @g_i64, align 8
+  %2 = srem i64 %0, %1
 
-  ret i64 %1
+  ret i64 %2
 }
 
 define i16 @remu() #0 {
 entry:
 ; CHECK: remu:
 
 ; CHECK: call #__mspabi_remu
   %0 = load volatile i16, i16* @g_i16, align 8
-  %1 = urem i16 %0, %0
+  %1 = load volatile i16, i16* @g_i16, align 8
+  %2 = urem i16 %0, %1
 
-  ret i16 %1
+  ret i16 %2
 }
 
 define i32 @remul() #0 {
 entry:
 ; CHECK: remul:
 
 ; CHECK: call #__mspabi_remul
   %0 = load volatile i32, i32* @g_i32, align 8
-  %1 = urem i32 %0, %0
+  %1 = load volatile i32, i32* @g_i32, align 8
+  %2 = urem i32 %0, %1
 
-  ret i32 %1
+  ret i32 %2
 }
 
 define i64 @remull() #0 {
 entry:
 ; CHECK: remull:
 
 ; CHECK: call #__mspabi_remull
   %0 = load volatile i64, i64* @g_i64, align 8
-  %1 = urem i64 %0, %0
+  %1 = load volatile i64, i64* @g_i64, align 8
+  %2 = urem i64 %0, %1
 
-  ret i64 %1
+  ret i64 %2
 }
 
 define i16 @mpyi() #0 {
 entry:
 ; CHECK: mpyi:
 
 ; CHECK: call #__mspabi_mpyi
   %0 = load volatile i16, i16* @g_i16, align 8

llvm/trunk/test/CodeGen/SystemZ/pr32372.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc %s -o - -mtriple=s390x-linux-gnu | FileCheck %s
 
 define void @pr32372(i8*) {
 ; CHECK-LABEL: pr32372:
 ; CHECK:       # %bb.0: # %BB
-; CHECK-NEXT:    llc %r1, 0(%r2)
 ; CHECK-NEXT:    mvhhi 0(%r1), -3825
-; CHECK-NEXT:    llill %r0, 0
-; CHECK-NEXT:    dlr %r0, %r1
 ; CHECK-NEXT:  .LBB0_1: # %CF251
 ; CHECK-NEXT:    # =>This Inner Loop Header: Depth=1
 ; CHECK-NEXT:    j .LBB0_1
 BB:
   %L = load i8, i8* %0
   store i16 -3825, i16* undef
   %L5 = load i8, i8* %0
-  %B9 = urem i8 %L5, %L
+  %B8 = call { i8, i1 } @llvm.umul.with.overflow.i8(i8 %L5, i8 %L)
+  %B9 = extractvalue {i8, i1} %B8, 0
   %I107 = insertelement <8 x i8> zeroinitializer, i8 %B9, i32 7
   %ZE141 = zext i8 %L5 to i16
   br label %CF251
 
 CF251:                                            ; preds = %CF258, %CF251, %BB
   %Shuff217 = shufflevector <8 x i8> zeroinitializer, <8 x i8> %I107, <8 x i32> <i32 0, i32 2, i32 undef, i32 6, i32 8, i32 undef, i32 12, i32 14>
   %Cmp227 = icmp sge i16 %ZE141, 0
   br i1 %Cmp227, label %CF251, label %CF258
 
 CF258:                                            ; preds = %CF251
   %Shuff230 = shufflevector <2 x i16> undef, <2 x i16> undef, <2 x i32> <i32 3, i32 1>
   br label %CF251
 }
+
+declare {i8, i1} @llvm.umul.with.overflow.i8(i8, i8) nounwind readnone

llvm/trunk/test/CodeGen/X86/pr38539.ll

 ; X64-NEXT:    shlq $32, %rsi
 ; X64-NEXT:    orq %rax, %rsi
 ; X64-NEXT:    movq %rsi, %rdi
 ; X64-NEXT:    shlq $30, %rdi
 ; X64-NEXT:    sarq $30, %rdi
 ; X64-NEXT:    movb (%rax), %al
 ; X64-NEXT:    movzbl %al, %eax
 ; X64-NEXT:    # kill: def $eax killed $eax def $ax
-; X64-NEXT:    divb %al
+; X64-NEXT:    divb (%rax)
 ; X64-NEXT:    movl %eax, %r8d
 ; X64-NEXT:    xorl %eax, %eax
 ; X64-NEXT:    xorl %edx, %edx
 ; X64-NEXT:    idivq %rdi
 ; X64-NEXT:    movabsq $17179869183, %rax # imm = 0x3FFFFFFFF
 ; X64-NEXT:    andq %rdx, %rax
 ; X64-NEXT:    testb %al, %al
 ; X64-NEXT:    setne %dil

 ; X86-NEXT:    pushl %edi
 ; X86-NEXT:    pushl %esi
 ; X86-NEXT:    andl $-8, %esp
 ; X86-NEXT:    subl $16, %esp
 ; X86-NEXT:    .cfi_offset %esi, -20
 ; X86-NEXT:    .cfi_offset %edi, -16
 ; X86-NEXT:    .cfi_offset %ebx, -12
 ; X86-NEXT:    movzbl {{[0-9]+}}(%esp), %esi
-; X86-NEXT:    movl %esi, %eax
-; X86-NEXT:    shll $30, %eax
-; X86-NEXT:    sarl $30, %eax
+; X86-NEXT:    movl %esi, %ecx
+; X86-NEXT:    shll $30, %ecx
+; X86-NEXT:    sarl $30, %ecx
 ; X86-NEXT:    movl (%esp), %edi
-; X86-NEXT:    movb (%eax), %bl
-; X86-NEXT:    pushl %eax
+; X86-NEXT:    movb (%eax), %al
+; X86-NEXT:    movzbl %al, %eax
+; X86-NEXT:    # kill: def $eax killed $eax def $ax
+; X86-NEXT:    divb (%eax)
+; X86-NEXT:    movl %eax, %ebx
+; X86-NEXT:    pushl %ecx
 ; X86-NEXT:    pushl %edi
 ; X86-NEXT:    pushl $0
 ; X86-NEXT:    pushl $0
 ; X86-NEXT:    calll __moddi3
 ; X86-NEXT:    addl $16, %esp
 ; X86-NEXT:    andl $3, %edx
+; X86-NEXT:    testb %al, %al
+; X86-NEXT:    setne (%eax)
 ; X86-NEXT:    cmpl %eax, %edi
 ; X86-NEXT:    sbbl %edx, %esi
-; X86-NEXT:    setb %dl
-; X86-NEXT:    setae %dh
+; X86-NEXT:    setae %dl
+; X86-NEXT:    sbbb %cl, %cl
 ; X86-NEXT:    testb %al, %al
-; X86-NEXT:    setne %bh
-; X86-NEXT:    setne (%eax)
-; X86-NEXT:    movzbl %bl, %eax
-; X86-NEXT:    xorl %ecx, %ecx
-; X86-NEXT:    subb %dl, %cl
-; X86-NEXT:    # kill: def $eax killed $eax def $ax
-; X86-NEXT:    divb %bl
-; X86-NEXT:    negb %dh
-; X86-NEXT:    cmpb %al, %al
+; X86-NEXT:    setne %ch
+; X86-NEXT:    negb %dl
+; X86-NEXT:    cmpb %bl, %al
 ; X86-NEXT:    setle %al
 ; X86-NEXT:    negb %al
 ; X86-NEXT:    cbtw
-; X86-NEXT:    idivb %dh
+; X86-NEXT:    idivb %dl
 ; X86-NEXT:    movsbl %ah, %eax
 ; X86-NEXT:    movzbl %al, %eax
 ; X86-NEXT:    andl $1, %eax
 ; X86-NEXT:    shll $3, %eax
 ; X86-NEXT:    negl %eax
-; X86-NEXT:    negb %bh
+; X86-NEXT:    negb %ch
 ; X86-NEXT:    leal -8(%esp,%eax), %eax
 ; X86-NEXT:    movl %eax, (%eax)
 ; X86-NEXT:    movl %ecx, %eax
 ; X86-NEXT:    cbtw
-; X86-NEXT:    idivb %bh
+; X86-NEXT:    idivb %ch
 ; X86-NEXT:    movsbl %ah, %eax
 ; X86-NEXT:    andb $1, %al
 ; X86-NEXT:    movb %al, (%eax)
 ; X86-NEXT:    leal -12(%ebp), %esp
 ; X86-NEXT:    popl %esi
 ; X86-NEXT:    popl %edi
 ; X86-NEXT:    popl %ebx
 ; X86-NEXT:    popl %ebp
 ; X86-NEXT:    .cfi_def_cfa %esp, 4
 ; X86-NEXT:    retl
 BB:
   %A30 = alloca i34
   %L17 = load i34, i34* %A30
   %B20 = and i34 %L17, -1
   %G2 = getelementptr i34, i34* %A30, i1 true
-  %L10 = load i8, i8* undef
-  %B6 = udiv i8 %L10, %L10
+  %L10 = load volatile i8, i8* undef
+  %L11 = load volatile i8, i8* undef
+  %B6 = udiv i8 %L10, %L11
   %C15 = icmp eq i8 undef, 0
   %B8 = srem i34 0, %B20
   %C2 = icmp ule i34 %B8, %B20
   %B5 = or i8 0, %B6
   %C19 = icmp uge i1 false, %C2
   %C1 = icmp sle i8 undef, %B5
   %B37 = srem i1 %C1, %C2
   %C7 = icmp uge i1 false, %C15
   store i1 %C7, i1* undef
   %G6 = getelementptr i34, i34* %G2, i1 %B37
   store i34* %G6, i34** undef
   %B30 = srem i1 %C19, %C7
   store i1 %B30, i1* undef
   ret void
 }