[X86][GlobalISel] Enable G_SDIV/G_UDIV/G_SREM/G_UREM #81615

e-kud · 2024-02-13T16:08:32Z

Create a libcall for s64 type for 32 bit targets.
Fix a bug in REM selection: SUBREG_TO_REG is not intended to produce a value from super registers.
Replace selector tests by end-to-end tests. Other passes check the selected MIR better.

* Create a libcall for s64 type for 32 bit targets. * Fix a bug in REM selection: SUBREG_TO_REG is not intended to produce a value from super registers. * Replace per-pass tests by end-to-end tests. They haven't revealed the aforementioned bug.

llvmbot · 2024-02-13T16:09:04Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-globalisel

Author: Evgenii Kudriashov (e-kud)

Changes

Create a libcall for s64 type for 32 bit targets.
Fix a bug in REM selection: SUBREG_TO_REG is not intended to produce a value from super registers.
Replace per-pass tests by end-to-end tests. They haven't revealed the aforementioned bug.

Patch is 130.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/81615.diff

22 Files Affected:

(modified) llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp (+3-6)
(modified) llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp (+1)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-legalize-sdiv.mir (-114)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-legalize-srem.mir (-211)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-legalize-udiv.mir (-195)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-legalize-urem.mir (-211)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-select-sdiv.mir (-130)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-select-srem.mir (-213)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-select-udiv.mir (-215)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86-select-urem.mir (-215)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-legalize-sdiv.mir (-145)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-legalize-srem.mir (-253)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-legalize-udiv.mir (-253)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-legalize-urem.mir (-253)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-select-sdiv.mir (-164)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-select-srem.mir (-270)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-select-udiv.mir (-267)
(removed) llvm/test/CodeGen/X86/GlobalISel/x86_64-select-urem.mir (-273)
(added) llvm/test/CodeGen/X86/isel-sdiv.ll (+116)
(added) llvm/test/CodeGen/X86/isel-srem.ll (+150)
(added) llvm/test/CodeGen/X86/isel-udiv.ll (+116)
(added) llvm/test/CodeGen/X86/isel-urem.ll (+150)

diff --git a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
index 26932ba2c8e242..8e0f61a855661b 100644
--- a/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
+++ b/llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp
@@ -1778,12 +1778,9 @@ bool X86InstructionSelector::selectMulDivRem(MachineInstr &I,
         .addImm(8);
 
     // Now reference the 8-bit subreg of the result.
-    BuildMI(*I.getParent(), I, I.getDebugLoc(),
-            TII.get(TargetOpcode::SUBREG_TO_REG))
-        .addDef(DstReg)
-        .addImm(0)
-        .addReg(ResultSuperReg)
-        .addImm(X86::sub_8bit);
+    BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(TargetOpcode::COPY),
+            DstReg)
+        .addReg(ResultSuperReg, 0, X86::sub_8bit);
   } else {
     BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(TargetOpcode::COPY),
             DstReg)
diff --git a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
index 27381dff338e2d..4e83753b67e7e5 100644
--- a/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
+++ b/llvm/lib/Target/X86/GISel/X86LegalizerInfo.cpp
@@ -212,6 +212,7 @@ X86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
         return typeInSet(0, {s8, s16, s32})(Query) ||
                (Is64Bit && typeInSet(0, {s64})(Query));
       })
+      .libcallFor({s64})
       .clampScalar(0, s8, sMaxScalar);
 
   // integer shifts
diff --git a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-sdiv.mir b/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-sdiv.mir
deleted file mode 100644
index 80382db942722c..00000000000000
--- a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-sdiv.mir
+++ /dev/null
@@ -1,114 +0,0 @@
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=i686-linux-gnu -run-pass=legalizer -verify-machineinstrs %s -o - | FileCheck %s
-
---- |
-  ; ModuleID = 'sdiv.ll'
-  source_filename = "sdiv.ll"
-  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
-
-  define i8 @test_sdiv_i8(i8 %arg1, i8 %arg2) {
-    %res = sdiv i8 %arg1, %arg2
-    ret i8 %res
-  }
-
-  define i16 @test_sdiv_i16(i16 %arg1, i16 %arg2) {
-    %res = sdiv i16 %arg1, %arg2
-    ret i16 %res
-  }
-
-  define i32 @test_sdiv_i32(i32 %arg1, i32 %arg2) {
-    %res = sdiv i32 %arg1, %arg2
-    ret i32 %res
-  }
-
-...
----
-name:            test_sdiv_i8
-alignment:       16
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _ }
-  - { id: 1, class: _ }
-  - { id: 2, class: _ }
-  - { id: 3, class: _ }
-  - { id: 4, class: _ }
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_sdiv_i8
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s8) = G_TRUNC [[COPY]](s32)
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[TRUNC1:%[0-9]+]]:_(s8) = G_TRUNC [[COPY1]](s32)
-    ; CHECK: [[SDIV:%[0-9]+]]:_(s8) = G_SDIV [[TRUNC]], [[TRUNC1]]
-    ; CHECK: $al = COPY [[SDIV]](s8)
-    ; CHECK: RET 0, implicit $al
-    %2:_(s32) = COPY $edi
-    %0:_(s8) = G_TRUNC %2(s32)
-    %3:_(s32) = COPY $esi
-    %1:_(s8) = G_TRUNC %3(s32)
-    %4:_(s8) = G_SDIV %0, %1
-    $al = COPY %4(s8)
-    RET 0, implicit $al
-
-...
----
-name:            test_sdiv_i16
-alignment:       16
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _ }
-  - { id: 1, class: _ }
-  - { id: 2, class: _ }
-  - { id: 3, class: _ }
-  - { id: 4, class: _ }
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_sdiv_i16
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY]](s32)
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[TRUNC1:%[0-9]+]]:_(s16) = G_TRUNC [[COPY1]](s32)
-    ; CHECK: [[SDIV:%[0-9]+]]:_(s16) = G_SDIV [[TRUNC]], [[TRUNC1]]
-    ; CHECK: $ax = COPY [[SDIV]](s16)
-    ; CHECK: RET 0, implicit $ax
-    %2:_(s32) = COPY $edi
-    %0:_(s16) = G_TRUNC %2(s32)
-    %3:_(s32) = COPY $esi
-    %1:_(s16) = G_TRUNC %3(s32)
-    %4:_(s16) = G_SDIV %0, %1
-    $ax = COPY %4(s16)
-    RET 0, implicit $ax
-
-...
----
-name:            test_sdiv_i32
-alignment:       16
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _ }
-  - { id: 1, class: _ }
-  - { id: 2, class: _ }
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_sdiv_i32
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[SDIV:%[0-9]+]]:_(s32) = G_SDIV [[COPY]], [[COPY1]]
-    ; CHECK: $eax = COPY [[SDIV]](s32)
-    ; CHECK: RET 0, implicit $eax
-    %0:_(s32) = COPY $edi
-    %1:_(s32) = COPY $esi
-    %2:_(s32) = G_SDIV %0, %1
-    $eax = COPY %2(s32)
-    RET 0, implicit $eax
-
-...
diff --git a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-srem.mir b/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-srem.mir
deleted file mode 100644
index 965bf635d6feb8..00000000000000
--- a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-srem.mir
+++ /dev/null
@@ -1,211 +0,0 @@
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=i686-linux-gnu -run-pass=legalizer -verify-machineinstrs %s -o - | FileCheck %s
-
---- |
-  ; ModuleID = 'srem.ll'
-  source_filename = "srem.ll"
-  target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
-  target triple = "i386--linux-gnu"
-
-  define i8 @test_srem_i8(i8 %arg1, i8 %arg2) {
-    %res = srem i8 %arg1, %arg2
-    ret i8 %res
-  }
-
-  define i16 @test_srem_i16(i16 %arg1, i16 %arg2) {
-    %res = srem i16 %arg1, %arg2
-    ret i16 %res
-  }
-
-  define i32 @test_srem_i32(i32 %arg1, i32 %arg2) {
-    %res = srem i32 %arg1, %arg2
-    ret i32 %res
-  }
-
-...
----
-name:            test_srem_i8
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    4
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-  - { id: 0, type: default, offset: 4, size: 1, alignment: 4, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-  - { id: 1, type: default, offset: 0, size: 1, alignment: 16, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    ; CHECK-LABEL: name: test_srem_i8
-    ; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    ; CHECK: [[LOAD:%[0-9]+]]:_(s8) = G_LOAD [[FRAME_INDEX]](p0) :: (invariant load (s8) from %fixed-stack.0, align 16)
-    ; CHECK: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s8) = G_LOAD [[FRAME_INDEX1]](p0) :: (invariant load (s8) from %fixed-stack.1, align 4)
-    ; CHECK: [[SREM:%[0-9]+]]:_(s8) = G_SREM [[LOAD]], [[LOAD1]]
-    ; CHECK: $al = COPY [[SREM]](s8)
-    ; CHECK: RET 0, implicit $al
-    %2:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    %0:_(s8) = G_LOAD %2(p0) :: (invariant load (s8) from %fixed-stack.1, align 16)
-    %3:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    %1:_(s8) = G_LOAD %3(p0) :: (invariant load (s8) from %fixed-stack.0, align 4)
-    %4:_(s8) = G_SREM %0, %1
-    $al = COPY %4(s8)
-    RET 0, implicit $al
-
-...
----
-name:            test_srem_i16
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    4
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-  - { id: 0, type: default, offset: 4, size: 2, alignment: 4, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-  - { id: 1, type: default, offset: 0, size: 2, alignment: 16, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    ; CHECK-LABEL: name: test_srem_i16
-    ; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    ; CHECK: [[LOAD:%[0-9]+]]:_(s16) = G_LOAD [[FRAME_INDEX]](p0) :: (invariant load (s16) from %fixed-stack.0, align 16)
-    ; CHECK: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s16) = G_LOAD [[FRAME_INDEX1]](p0) :: (invariant load (s16) from %fixed-stack.1, align 4)
-    ; CHECK: [[SREM:%[0-9]+]]:_(s16) = G_SREM [[LOAD]], [[LOAD1]]
-    ; CHECK: $ax = COPY [[SREM]](s16)
-    ; CHECK: RET 0, implicit $ax
-    %2:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    %0:_(s16) = G_LOAD %2(p0) :: (invariant load (s16) from %fixed-stack.1, align 16)
-    %3:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    %1:_(s16) = G_LOAD %3(p0) :: (invariant load (s16) from %fixed-stack.0, align 4)
-    %4:_(s16) = G_SREM %0, %1
-    $ax = COPY %4(s16)
-    RET 0, implicit $ax
-
-...
----
-name:            test_srem_i32
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    4
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-  - { id: 0, type: default, offset: 4, size: 4, alignment: 4, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-  - { id: 1, type: default, offset: 0, size: 4, alignment: 16, stack-id: default,
-      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
-      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    ; CHECK-LABEL: name: test_srem_i32
-    ; CHECK: [[FRAME_INDEX:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    ; CHECK: [[LOAD:%[0-9]+]]:_(s32) = G_LOAD [[FRAME_INDEX]](p0) :: (invariant load (s32) from %fixed-stack.0, align 16)
-    ; CHECK: [[FRAME_INDEX1:%[0-9]+]]:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    ; CHECK: [[LOAD1:%[0-9]+]]:_(s32) = G_LOAD [[FRAME_INDEX1]](p0) :: (invariant load (s32) from %fixed-stack.1)
-    ; CHECK: [[SREM:%[0-9]+]]:_(s32) = G_SREM [[LOAD]], [[LOAD1]]
-    ; CHECK: $eax = COPY [[SREM]](s32)
-    ; CHECK: RET 0, implicit $eax
-    %2:_(p0) = G_FRAME_INDEX %fixed-stack.1
-    %0:_(s32) = G_LOAD %2(p0) :: (invariant load (s32) from %fixed-stack.1, align 16)
-    %3:_(p0) = G_FRAME_INDEX %fixed-stack.0
-    %1:_(s32) = G_LOAD %3(p0) :: (invariant load (s32) from %fixed-stack.0, align 4)
-    %4:_(s32) = G_SREM %0, %1
-    $eax = COPY %4(s32)
-    RET 0, implicit $eax
-
-...
diff --git a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-udiv.mir b/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-udiv.mir
deleted file mode 100644
index 85c9b6d9e86bfd..00000000000000
--- a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-udiv.mir
+++ /dev/null
@@ -1,195 +0,0 @@
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=i686-linux-gnu -run-pass=legalizer -verify-machineinstrs %s -o - | FileCheck %s
-
---- |
-  ; ModuleID = 'udiv.ll'
-  source_filename = "udiv.ll"
-  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
-
-  define i8 @test_udiv_i8(i8 %arg1, i8 %arg2) {
-    %res = udiv i8 %arg1, %arg2
-    ret i8 %res
-  }
-
-  define i16 @test_udiv_i16(i16 %arg1, i16 %arg2) {
-    %res = udiv i16 %arg1, %arg2
-    ret i16 %res
-  }
-
-  define i32 @test_udiv_i32(i32 %arg1, i32 %arg2) {
-    %res = udiv i32 %arg1, %arg2
-    ret i32 %res
-  }
-
-...
----
-name:            test_udiv_i8
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    0
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_udiv_i8
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s8) = G_TRUNC [[COPY]](s32)
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[TRUNC1:%[0-9]+]]:_(s8) = G_TRUNC [[COPY1]](s32)
-    ; CHECK: [[UDIV:%[0-9]+]]:_(s8) = G_UDIV [[TRUNC]], [[TRUNC1]]
-    ; CHECK: $al = COPY [[UDIV]](s8)
-    ; CHECK: RET 0, implicit $al
-    %2:_(s32) = COPY $edi
-    %0:_(s8) = G_TRUNC %2(s32)
-    %3:_(s32) = COPY $esi
-    %1:_(s8) = G_TRUNC %3(s32)
-    %4:_(s8) = G_UDIV %0, %1
-    $al = COPY %4(s8)
-    RET 0, implicit $al
-
-...
----
-name:            test_udiv_i16
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    0
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_udiv_i16
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[TRUNC:%[0-9]+]]:_(s16) = G_TRUNC [[COPY]](s32)
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[TRUNC1:%[0-9]+]]:_(s16) = G_TRUNC [[COPY1]](s32)
-    ; CHECK: [[UDIV:%[0-9]+]]:_(s16) = G_UDIV [[TRUNC]], [[TRUNC1]]
-    ; CHECK: $ax = COPY [[UDIV]](s16)
-    ; CHECK: RET 0, implicit $ax
-    %2:_(s32) = COPY $edi
-    %0:_(s16) = G_TRUNC %2(s32)
-    %3:_(s32) = COPY $esi
-    %1:_(s16) = G_TRUNC %3(s32)
-    %4:_(s16) = G_UDIV %0, %1
-    $ax = COPY %4(s16)
-    RET 0, implicit $ax
-
-...
----
-name:            test_udiv_i32
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-liveins:
-frameInfo:
-  isFrameAddressTaken: false
-  isReturnAddressTaken: false
-  hasStackMap:     false
-  hasPatchPoint:   false
-  stackSize:       0
-  offsetAdjustment: 0
-  maxAlignment:    0
-  adjustsStack:    false
-  hasCalls:        false
-  stackProtector:  ''
-  maxCallFrameSize: 4294967295
-  hasOpaqueSPAdjustment: false
-  hasVAStart:      false
-  hasMustTailInVarArgFunc: false
-  localFrameSize:  0
-  savePoint:       ''
-  restorePoint:    ''
-fixedStack:
-stack:
-constants:
-body:             |
-  bb.1 (%ir-block.0):
-    liveins: $edi, $esi
-
-    ; CHECK-LABEL: name: test_udiv_i32
-    ; CHECK: liveins: $edi, $esi
-    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $edi
-    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $esi
-    ; CHECK: [[UDIV:%[0-9]+]]:_(s32) = G_UDIV [[COPY]], [[COPY1]]
-    ; CHECK: $eax = COPY [[UDIV]](s32)
-    ; CHECK: RET 0, implicit $eax
-    %0:_(s32) = COPY $edi
-    %1:_(s32) = COPY $esi
-    %2:_(s32) = G_UDIV %0, %1
-    $eax = COPY %2(s32)
-    RET 0, implicit $eax
-
-...
diff --git a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-urem.mir b/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-urem.mir
deleted file mode 100644
index b6496216ac56da..00000000000000
--- a/llvm/test/CodeGen/X86/GlobalISel/x86-legalize-urem.mir
+++ /dev/null
@@ -1,211 +0,0 @@
-# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
-# RUN: llc -mtriple=i686-linux-gnu -run-pass=legalizer -verify-machineinstrs %s -o - | FileCheck %s
-
---- |
-  ; ModuleID = 'urem.ll'
-  source_filename = "urem.ll"
-  target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
-  target triple = "i386--linux-gnu"
-
-  define i8 @test_urem_i8(i8 %arg1, i8 %arg2) {
-    %res = urem i8 %arg1, %arg2
-    ret i8 %res
-  }
-
-  define i16 @test_urem_i16(i16 %arg1, i16 %arg2) {
-    %res = urem i16 %arg1, %arg2
-    ret i16 %res
-  }
-
-  define i32 @test_urem_i32(i32 %arg1, i32 %arg2) {
-    %res = urem i32 %arg1, %arg2
-    ret i32 %res
-  }
-
-...
----
-name:            test_urem_i8
-alignment:       16
-exposesReturnsTwice: false
-legalized:       false
-regBankSelected: false
-selected:        false
-failedISel:      false
-tracksRegLiveness: true
-registers:
-  - { id: 0, class: _, preferred-register: '' }
-  - { id: 1, class: _, preferred-register: '' }
-  - { id: 2, class: _, preferred-register: '' }
-  - { id: 3, class: _, preferred-register: '' }
-  - { id: 4, class: _, preferred-register: '' }
-liveins:
-fr...
[truncated]

tschuett · 2024-02-13T18:01:34Z

One of the nice features of GlobalIsel is that you can test passes in isolation. Why did the mir tests did not reveal the bug?
End-to-end tests are a SelectionDag feature.

arsenm · 2024-02-13T18:36:52Z

One of the nice features of GlobalIsel is that you can test passes in isolation. Why did the mir tests did not reveal the bug? End-to-end tests are a SelectionDag feature.

Neither is a replacement for each other. I would add new end to end tests, not delete existing MIR tests

tschuett · 2024-02-13T18:43:19Z

I would also amend the select mir files to show the fixed bug.

topperc · 2024-02-13T20:43:28Z

One of the nice features of GlobalIsel is that you can test passes in isolation. Why did the mir tests did not reveal the bug? End-to-end tests are a SelectionDag feature.

They show the SUBREG_TO_REG, but SUBREG_TO_REG is often misunderstood and there's nothing that can check that is correctly used.

topperc · 2024-02-13T20:46:56Z

llvm/lib/Target/X86/GISel/X86InstructionSelector.cpp

@@ -1778,12 +1778,9 @@ bool X86InstructionSelector::selectMulDivRem(MachineInstr &I,
        .addImm(8);


Probably for another patch, but we should match SelectionDAG here and use a MOVZX_NOREX or MOVSX_NOREX from AH here.

Do you mean to replace COPY + SHR by movzx %ah, %ax?

Yes this is the code from X86ISelDAGToDAG.cpp

if (HiReg == X86::AH && !SDValue(Node, 1).use_empty()) { SDValue AHCopy = CurDAG->getRegister(X86::AH, MVT::i8); unsigned AHExtOpcode = isSigned ? X86::MOVSX32rr8_NOREX : X86::MOVZX32rr8_NOREX; SDNode *RNode = CurDAG->getMachineNode(AHExtOpcode, dl, MVT::i32, MVT::Glue, AHCopy, InGlue); SDValue Result(RNode, 0); InGlue = SDValue(RNode, 1); Result = CurDAG->getTargetExtractSubreg(X86::sub_8bit, dl, MVT::i8, Result); ReplaceUses(SDValue(Node, 1), Result); LLVM_DEBUG(dbgs() << "=> "; Result.getNode()->dump(CurDAG); dbgs() << '\n'); }

RKSimon

Should we be losing the legalize tests (although maybe we should be merging 32/64 bit targets - no i64 divrem on x86 tests atm)? The select tests I don't find as much use for (and the isel-* tests are better to identify poor codegen IMO)

arsenm · 2024-02-14T11:55:07Z

They show the SUBREG_TO_REG, but SUBREG_TO_REG is often misunderstood and there's nothing that can check that is correctly used.

We really should just remove SUBREG_TO_REG, it's impossible to use it correctly. My patch to hack around it in the coalescer is currently stuck on it's 5th or 6th revert due to unrelated issues it happens to hit

e-kud · 2024-02-20T00:04:14Z

I want to admit that it was too harsh to replace all mir tests with ll tests. Returned and unified legalizer tests as @RKSimon suggested.

@tschuett do you think we still need updated select tests? These tests won't be much different from the existing ones that check invalid MIR. I think that the best check of instruction selection is valid final assembly. But I don't mind to keep them if we see their usefulness. Before fix it was an assert in register allocator due to SUBREG_TO_REG misuse.

e-kud · 2024-02-20T00:05:51Z

They show the SUBREG_TO_REG, but SUBREG_TO_REG is often misunderstood and there's nothing that can check that is correctly used.

@topperc maybe we can add a subreg-superreg check to MachineVerifier?

arsenm · 2024-02-28T06:20:03Z

@topperc maybe we can add a subreg-superreg check to MachineVerifier?

Not really, the operation is more fundamentally broken. It is promising something about bits in registers which are not represented in the instruction

arsenm

lgtm with nits

arsenm · 2024-02-28T06:20:23Z

llvm/test/CodeGen/X86/GlobalISel/legalize-sdiv.mir

+# RUN: llc -mtriple=i686-linux-gnu -run-pass=legalizer -verify-machineinstrs %s -o - | FileCheck %s --check-prefixes=CHECK,X86
+
+--- |
+  source_filename = "sdiv.ll"


Don't need the IR section

I've always thought that IR section is needed to regenerate MIR if something has changed. Or is it not a common practice?

No, it's needed for a specific set of IR references (block names, IR values in MMOs, and global references)

llvm/test/CodeGen/X86/GlobalISel/legalize-sdiv.mir

arsenm · 2024-02-28T06:21:01Z

llvm/test/CodeGen/X86/isel-sdiv.ll

+; RUN: llc < %s -fast-isel -fast-isel-abort=1     -mtriple=i686-linux-gnu | FileCheck %s --check-prefixes=X86,DAG-X86
+; RUN: llc < %s -global-isel -global-isel-abort=1 -mtriple=i686-linux-gnu | FileCheck %s --check-prefixes=X86,GISEL-X86
+
+define i8 @test_sdiv_i8(i8 %arg1, i8 %arg2) nounwind {


Maybe want vector tests somewhere too

FTR - I'd prefer vector tests to be in separate files as there is a lot of SSE/AVX level bloat

@arsenm the vector work is in parallel with scalar instruction. We've started with some basics G_BUILD_VECTOR and G_INSERT/EXTRACT_VECTOR_ELT. We'll reiterate all operations ones more with adding tests for vector operands.

@RKSimon I agree with splitting vector/scalar cases, as well as float/scalar in some situations. I'm mostly motivated by creating a generic testsuite that can be reused for other targets when they start adding GlobalISel support incrementally. I'm slightly struggling with existing mixed vector/float/int tests that I need to split for GlobalISel testing as not everything's implemented or to duplicate them.

However maybe this is not feasible as after some time there will be tests for non-standard sizes like i42 that nobody will support at early stages.

e-kud requested a review from RKSimon February 13, 2024 16:08

llvmbot added backend:X86 llvm:globalisel labels Feb 13, 2024

topperc reviewed Feb 13, 2024

View reviewed changes

RKSimon reviewed Feb 14, 2024

View reviewed changes

Unify MIR legalizer tests

aec6cd2

arsenm approved these changes Feb 28, 2024

View reviewed changes

e-kud added 3 commits February 28, 2024 16:32

Merge remote-tracking branch 'origin/main' into global-div-rem

30d4578

Remove alignment and registers from mir tests

07459c4

Drop IR from mir tests

f871f65

arsenm approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into global-div-rem

e0c884c

e-kud merged commit 10edabb into llvm:main Mar 7, 2024
3 of 4 checks passed

e-kud deleted the global-div-rem branch March 7, 2024 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[X86][GlobalISel] Enable G_SDIV/G_UDIV/G_SREM/G_UREM #81615

[X86][GlobalISel] Enable G_SDIV/G_UDIV/G_SREM/G_UREM #81615

e-kud commented Feb 13, 2024 •

edited

llvmbot commented Feb 13, 2024 •

edited

tschuett commented Feb 13, 2024

arsenm commented Feb 13, 2024

tschuett commented Feb 13, 2024

topperc commented Feb 13, 2024

topperc Feb 13, 2024

e-kud Feb 20, 2024

topperc Feb 20, 2024

RKSimon left a comment

arsenm commented Feb 14, 2024

e-kud commented Feb 20, 2024

e-kud commented Feb 20, 2024

arsenm commented Feb 28, 2024

arsenm left a comment

arsenm Feb 28, 2024

e-kud Feb 28, 2024

arsenm Feb 29, 2024

arsenm Feb 28, 2024

RKSimon Feb 28, 2024

e-kud Feb 28, 2024

		@@ -1778,12 +1778,9 @@ bool X86InstructionSelector::selectMulDivRem(MachineInstr &I,
		.addImm(8);

[X86][GlobalISel] Enable G_SDIV/G_UDIV/G_SREM/G_UREM #81615

[X86][GlobalISel] Enable G_SDIV/G_UDIV/G_SREM/G_UREM #81615

Conversation

e-kud commented Feb 13, 2024 • edited

llvmbot commented Feb 13, 2024 • edited

tschuett commented Feb 13, 2024

arsenm commented Feb 13, 2024

tschuett commented Feb 13, 2024

topperc commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RKSimon left a comment

Choose a reason for hiding this comment

arsenm commented Feb 14, 2024

e-kud commented Feb 20, 2024

e-kud commented Feb 20, 2024

arsenm commented Feb 28, 2024

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

e-kud commented Feb 13, 2024 •

edited

llvmbot commented Feb 13, 2024 •

edited