SelectionDAG: Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486.

[uabelho]

Summary

Originally

i64 = umax t8, Constant:i64<4>

was expanded into

i32,i32 = umax Constant:i32<0>, Constant:i32<0>
i32,i32 = umax t7, Constant:i32<4>

Now instead the two produced umax:es return i32 instead of i32, i32.

Thanks to Jan Vesely for help with the test case.

[arsenm]

test/CodeGen/AMDGPU/legalize-umax-bug.mir

1–2

Having a MIR test for a SelectionDAG bug doesn't really make sense. This should be a regular IR test.

Also can you run opt -instnamer on it?

[uabelho]

test/CodeGen/AMDGPU/legalize-umax-bug.mir

1–2

I agree, and on my out-of-tree target I get the crash from an ll-file. However, running that on -march=r600 -mcpu=cypress Early CSE kicks in and simplifies the test case so the crash doesn't appear anymore, and I can't find any flag to turn off Early CSE while still running later optimizations on the DAG?

So using the mir file (without any mir in it) was just a way to be able to start compilation after Early CSE, so it wouldn't mess up things for me.

Someone with better knowledge of the different in-tree target can probably reproduce the crash from an ll file but I fail.

I don't know what opt -instnamer is but I'll look into it.

[uabelho]

test/CodeGen/AMDGPU/legalize-umax-bug.mir

1–2

And, looking in my inbox I saw that I already had a better (ll) reproducer sent to me from Jan Vesely. Thanks! I'll update the patch!

(Also I learned that I can use -start-after also on ll files. For some reason I thought that didn't work...)

[uabelho]

Updated the test case.

[jvesely]

test/CodeGen/AMDGPU/legalize-umax-bug.ll

1 ↗

(On Diff #82651)

Can you prefix the test file name with r600. ?
I might also be a good idea use FileCheck and check that MAX_UINT was selected

[uabelho]

Prefixed the test case name with r600-.

[uabelho]

There were more test cases already prefixed with "r600-" than with "r600." so I chose "r600-". I'll change if "r600." is more correct?

As for FileCheck, sure I can add some checks. The output is

.text
.section        .AMDGPU.config
.long   166100
.long   2
.long   165900
.long   0
.long   166120
.long   0
.text
.globl  test
.p2align        8
.type   test,@function

test: ; @test
; BB#0: ; %bb

ALU 4, @6, KC0[CB0:0-32], KC1[]
MEM_RAT_CACHELESS STORE_RAW T0.XY, T1.X, 0
ALU 3, @11, KC0[], KC1[]
MEM_RAT_CACHELESS STORE_RAW T0.XY, T1.X, 1
CF_END
PAD
ALU clause starting at 6:
  MOV   T0.X, literal.x,  
  MOV   T0.Y, 0.0,  
  LSHR * T1.X, KC0[2].Y, literal.x,  
2(2.802597e-45), 0(0.000000e+00)
  MOV * T0.W, KC0[2].Y,  
ALU clause starting at 11:
  MAX_UINT   T0.X, T0.X, literal.x,  
  MOV   T0.Y, 0.0,  
  LSHR * T1.X, T0.W, literal.y,  
4(5.605194e-45), 2(2.802597e-45)

.Lfunc_end0:

.size   test, .Lfunc_end0-test

.section        .AMDGPU.csdata
;SQ_PGM_RESOURCES:STACK_SIZE = 0

.section        ".note.GNU-stack"

What CHECKs should I add?

[arsenm]

Checking just CHECK: MAX_UINT is probably fine

test/CodeGen/AMDGPU/r600-legalize-umax-bug.ll

4–5 ↗

(On Diff #83015)

You can remove these since they are redundant with the command line arguments

[uabelho]

Added CHECK in test case

[uabelho]

Anything else?

test/CodeGen/AMDGPU/r600-legalize-umax-bug.ll

4–5 ↗

(On Diff #83015)

Done.

[arsenm]

LGTM

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

void DAGTypeLegalizer::ExpandIntRes_MINMAX(SDNode *N,
   GetExpandedInteger(N->getOperand(0), LHSL, LHSH);
   GetExpandedInteger(N->getOperand(1), RHSL, RHSH);
 
   // Value types
   EVT NVT = LHSL.getValueType();
   EVT CCT = getSetCCResultType(NVT);
 
   // Hi part is always the same op
-  Hi = DAG.getNode(N->getOpcode(), DL, {NVT, NVT}, {LHSH, RHSH});
+  Hi = DAG.getNode(N->getOpcode(), DL, NVT, {LHSH, RHSH});
 
   // We need to know whether to select Lo part that corresponds to 'winning'
   // Hi part or if Hi parts are equal.
   SDValue IsHiLeft = DAG.getSetCC(DL, CCT, LHSH, RHSH, CondC);
   SDValue IsHiEq = DAG.getSetCC(DL, CCT, LHSH, RHSH, ISD::SETEQ);
 
   // Lo part corresponding to the 'winning' Hi part
   SDValue LoCmp = DAG.getSelect(DL, NVT, IsHiLeft, LHSL, RHSL);
 
   // Recursed Lo part if Hi parts are equal, this uses unsigned version
-  SDValue LoMinMax = DAG.getNode(LoOpc, DL, {NVT, NVT}, {LHSL, RHSL});
+  SDValue LoMinMax = DAG.getNode(LoOpc, DL, NVT, {LHSL, RHSL});
 
   Lo = DAG.getSelect(DL, NVT, IsHiEq, LoMinMax, LoCmp);
 }
 
 void DAGTypeLegalizer::ExpandIntRes_ADDSUB(SDNode *N,
                                            SDValue &Lo, SDValue &Hi) {
   SDLoc dl(N);
   // Expand the subcomponents.

test/CodeGen/AMDGPU/legalize-umax-bug.mir

+# RUN: llc -march=r600 -mcpu=cypress -start-after safe-stack %s -o /dev/null
+
+# Don't crash
+
+--- |
+  target datalayout = "e-p:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+
+  @func_37.foo = internal unnamed_addr addrspace(3) global [256 x i64] undef
+
+  define void @func_37() #0 {
+    %1 = call i32 @llvm.r600.read.local.size.y(), !range !0
+    %2 = call i32 @llvm.r600.read.local.size.z(), !range !0
+    %3 = call i32 @llvm.r600.read.tidig.x(), !range !0
+    %4 = call i32 @llvm.r600.read.tidig.y(), !range !0
+    %5 = call i32 @llvm.r600.read.tidig.z(), !range !0
+    %6 = mul nuw nsw i32 %1, %2
+    %7 = mul i32 %6, %3
+    %8 = mul nuw nsw i32 %4, %2
+    %9 = add i32 %7, %8
+    %10 = add i32 %9, %5
+    %11 = getelementptr inbounds [256 x i64], [256 x i64] addrspace(3)* @func_37.foo, i32 0, i32 %10
+    br label %bb9.preheader
+
+  bb9.preheader:                                    ; preds = %0
+    store i64 2, i64 addrspace(3)* %11
+    %.promoted = load i64, i64 addrspace(3)* %11
+    %12 = add i64 %.promoted, 1
+    br label %bb14
+
+  bb14:                                             ; preds = %bb14, %bb9.preheader
+    %_tmp17464 = icmp ult i64 undef, 4
+    %13 = xor i1 %_tmp17464, true
+    br i1 %13, label %bb11, label %bb14
+
+  bb11:                                             ; preds = %bb14
+    %14 = icmp ugt i64 %12, 4
+    %umax = select i1 %14, i64 %12, i64 4
+    store i64 %umax, i64* undef
+    ret void
+  }
+
+  ; Function Attrs: nounwind readnone
+  declare i32 @llvm.r600.read.local.size.y() #1
+
+  ; Function Attrs: nounwind readnone
+  declare i32 @llvm.r600.read.local.size.z() #1
+
+  ; Function Attrs: nounwind readnone
+  declare i32 @llvm.r600.read.tidig.x() #1
+
+  ; Function Attrs: nounwind readnone
+  declare i32 @llvm.r600.read.tidig.y() #1
+
+  ; Function Attrs: nounwind readnone
+  declare i32 @llvm.r600.read.tidig.z() #1
+
+  attributes #0 = { "target-cpu"="cypress" }
+  attributes #1 = { nounwind readnone }
+
+  !0 = !{i32 0, i32 2048}
+
+...
+---
+name:            func_37
+alignment:       0
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+body:             |
+  bb.0 (%ir-block.0):
+    RETURN
+...