This is an archive of the discontinued LLVM Phabricator instance.

Differential D59543

[AMDGPU] Ban non-supported min3/max3 promotions.
ClosedPublic

Authored by sheredom on Mar 19 2019, 6:28 AM.

Details

Reviewers
arsenm
nhaehnle
Commits
rGe85f6bd64fbe: [AMDGPU] Ban i8 min3 promotion.
rL356464: [AMDGPU] Ban i8 min3 promotion.
Summary

I found this really weird WWM-related case whereby through the WWM transformations our isel lowering was trying to promote 2 min's into a min3 for the i8 type, which our hardware doesn't support.

The new min3_i8.ll test case would previously spew the error:

PromoteIntegerResult #0: t69: i8 = SMIN3 t70, Constant:i8<0>, t68

Before the simple fix to our isel lowering to not do it for i8 MVT's.

Diff Detail

Event Timeline

sheredom created this revision.Mar 19 2019, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 6:28 AM
Herald added subscribers: llvm-commits, t-tye, tpr and 5 others. · View Herald Transcript
arsenm added inline comments.Mar 19 2019, 6:47 AM
lib/Target/AMDGPU/SIISelLowering.cpp
8137

It seems like blacklisting specific MVTs won't work

test/CodeGen/AMDGPU/min3_i8.ll
4 ↗(On Diff #191282)

I doubt we need a new test for this. Can you just add the trivial cases to test/CodeGen/AMDGPU/min3? (and the corresponding max3). Also should have some weird types, like i7, and i33

sheredom marked an inline comment as done.Mar 19 2019, 6:59 AM
sheredom added inline comments.
test/CodeGen/AMDGPU/min3_i8.ll
4 ↗(On Diff #191282)

Happy to add more test cases there - but when I tried the simple thing here LLVM was promoting the i8's -> higher types early and thus not encountering the bug.

To actually get the weird bug I found you needed every single statement in the .ll here, which is crazy because a bunch of the statements don't even take part in the operation.

If you think I should just remove this test case though it's your call.

sheredom updated this revision to Diff 191295.Mar 19 2019, 7:17 AM

Fixed review comments by Matt and added the extra test cases that were a great idea!

arsenm added inline comments.Mar 19 2019, 7:18 AM
test/CodeGen/AMDGPU/min3_i8.ll
4 ↗(On Diff #191282)

A test is definitely necessary. You should be able to reproduce this with a simpler test. You definitely shouldn't need all of this complex WWM stuff to do it

sheredom updated this revision to Diff 191298.Mar 19 2019, 7:24 AM

Changed it to a whitelist of types rather than a blacklist (much better idea).

sheredom marked 2 inline comments as done.Mar 19 2019, 7:25 AM
sheredom added inline comments.
lib/Target/AMDGPU/SIISelLowering.cpp
8137

Good idea, changed it to a whitelist!

sheredom marked an inline comment as done.Mar 19 2019, 7:25 AM
arsenm added a comment.Mar 19 2019, 7:34 AM

The simple test reproduces the issue I would expect:
PromoteIntegerResult #0: t62: i8 = UMIN3 t66, t63, t59

Do not know how to promote this operator!
UNREACHABLE executed at ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:54

So the simple test seems fine

test/CodeGen/AMDGPU/fmin3.ll
121–123

fmin isn't impacted by this change, but this doesn't hurt

test/CodeGen/AMDGPU/min3.ll
161

Max coverage would also be good, not just min

sheredom updated this revision to Diff 191302.Mar 19 2019, 7:41 AM
sheredom retitled this revision from [AMDGPU] Ban i8 min3 promotion. to [AMDGPU] Ban non-supported min3/max3 promotions..

Added the max3 cases too.

sheredom marked 3 inline comments as done.Mar 19 2019, 7:42 AM
sheredom added inline comments.
test/CodeGen/AMDGPU/min3.ll
161

Good shout - added!

sheredom marked an inline comment as done.Mar 19 2019, 7:42 AM
arsenm accepted this revision.Mar 19 2019, 8:09 AM

LGTM

This revision is now accepted and ready to land.Mar 19 2019, 8:09 AM
Closed by commit rL356464: [AMDGPU] Ban i8 min3 promotion. (authored by sheredom). · Explain WhyMar 19 2019, 8:49 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Target/
AMDGPU/
3 lines
test/
CodeGen/
AMDGPU/
26 lines
172 lines

Diff 191295

lib/Target/AMDGPU/SIISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 8,127 Lines • ▼ Show 20 LinesSDValue SITargetLowering::performMinMaxCombine(SDNode *N,
EVT VT = N->getValueType(0); EVT VT = N->getValueType(0);
unsigned Opc = N->getOpcode(); unsigned Opc = N->getOpcode();
SDValue Op0 = N->getOperand(0); SDValue Op0 = N->getOperand(0);
SDValue Op1 = N->getOperand(1); SDValue Op1 = N->getOperand(1);
// Only do this if the inner op has one use since this will just increases // Only do this if the inner op has one use since this will just increases
// register pressure for no benefit. // register pressure for no benefit.
if (Opc != AMDGPUISD::FMIN_LEGACY && Opc != AMDGPUISD::FMAX_LEGACY && if (Opc != AMDGPUISD::FMIN_LEGACY && Opc != AMDGPUISD::FMAX_LEGACY &&
!VT.isVector() && VT != MVT::f64 && !VT.isVector() && VT.isSimple() && VT != MVT::f64 && VT != MVT::i8 &&
arsenmUnsubmitted
Done
ReplyInline Actions

It seems like blacklisting specific MVTs won't work

arsenm: It seems like blacklisting specific MVTs won't work
sheredomAuthorUnsubmitted
Done
ReplyInline Actions

Good idea, changed it to a whitelist!

sheredom: Good idea, changed it to a whitelist!
((VT != MVT::f16 && VT != MVT::i16) || Subtarget->hasMin3Max3_16())) { ((VT != MVT::f16 && VT != MVT::i16) || Subtarget->hasMin3Max3_16())) {
// max(max(a, b), c) -> max3(a, b, c) // max(max(a, b), c) -> max3(a, b, c)
// min(min(a, b), c) -> min3(a, b, c) // min(min(a, b), c) -> min3(a, b, c)
if (Op0.getOpcode() == Opc && Op0.hasOneUse()) { if (Op0.getOpcode() == Opc && Op0.hasOneUse()) {
SDLoc DL(N); SDLoc DL(N);
return DAG.getNode(minMaxOpcToMin3Max3Opc(Opc), return DAG.getNode(minMaxOpcToMin3Max3Opc(Opc),
DL, DL,
N->getValueType(0), N->getValueType(0),
▲ Show 20 Lines • Show All 1,630 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/fmin3.ll

Show First 20 Lines • Show All 112 Lines • ▼ Show 20 Lines
define <2 x half> @no_fmin3_v2f16(<2 x half> %a, <2 x half> %b, <2 x half> %c, <2 x half> %d) #2 { define <2 x half> @no_fmin3_v2f16(<2 x half> %a, <2 x half> %b, <2 x half> %c, <2 x half> %d) #2 {
entry: entry:
%min = call <2 x half> @llvm.minnum.v2f16(<2 x half> %a, <2 x half> %b) %min = call <2 x half> @llvm.minnum.v2f16(<2 x half> %a, <2 x half> %b)
%min1 = call <2 x half> @llvm.minnum.v2f16(<2 x half> %c, <2 x half> %min) %min1 = call <2 x half> @llvm.minnum.v2f16(<2 x half> %c, <2 x half> %min)
%res = call <2 x half> @llvm.minnum.v2f16(<2 x half> %min1, <2 x half> %d) %res = call <2 x half> @llvm.minnum.v2f16(<2 x half> %min1, <2 x half> %d)
ret <2 x half> %res ret <2 x half> %res
} }
; GCN-LABEL: {{^}}test_fmin3_olt_0_f64:
; GCN-NOT: v_min3
define amdgpu_kernel void @test_fmin3_olt_0_f64(double addrspace(1)* %out, double addrspace(1)* %aptr, double addrspace(1)* %bptr, double addrspace(1)* %cptr) #0 {
arsenmUnsubmitted
Done
ReplyInline Actions

fmin isn't impacted by this change, but this doesn't hurt

arsenm: fmin isn't impacted by this change, but this doesn't hurt
%a = load volatile double, double addrspace(1)* %aptr, align 4
%b = load volatile double, double addrspace(1)* %bptr, align 4
%c = load volatile double, double addrspace(1)* %cptr, align 4
%f0 = call double @llvm.minnum.f64(double %a, double %b)
%f1 = call double @llvm.minnum.f64(double %f0, double %c)
store double %f1, double addrspace(1)* %out, align 4
ret void
}
; Commute operand of second fmin
; GCN-LABEL: {{^}}test_fmin3_olt_1_f64:
; GCN-NOT: v_min3
define amdgpu_kernel void @test_fmin3_olt_1_f64(double addrspace(1)* %out, double addrspace(1)* %aptr, double addrspace(1)* %bptr, double addrspace(1)* %cptr) #0 {
%a = load volatile double, double addrspace(1)* %aptr, align 4
%b = load volatile double, double addrspace(1)* %bptr, align 4
%c = load volatile double, double addrspace(1)* %cptr, align 4
%f0 = call double @llvm.minnum.f64(double %a, double %b)
%f1 = call double @llvm.minnum.f64(double %c, double %f0)
store double %f1, double addrspace(1)* %out, align 4
ret void
}
declare i32 @llvm.amdgcn.workitem.id.x() #1 declare i32 @llvm.amdgcn.workitem.id.x() #1
declare double @llvm.minnum.f64(double, double) #1
declare float @llvm.minnum.f32(float, float) #1 declare float @llvm.minnum.f32(float, float) #1
declare half @llvm.minnum.f16(half, half) #1 declare half @llvm.minnum.f16(half, half) #1
declare <2 x half> @llvm.minnum.v2f16(<2 x half>, <2 x half>) declare <2 x half> @llvm.minnum.v2f16(<2 x half>, <2 x half>)
attributes #0 = { nounwind } attributes #0 = { nounwind }
attributes #1 = { nounwind readnone speculatable } attributes #1 = { nounwind readnone speculatable }
attributes #2 = { nounwind "no-nans-fp-math"="true" } attributes #2 = { nounwind "no-nans-fp-math"="true" }

test/CodeGen/AMDGPU/min3.ll

Show First 20 Lines • Show All 152 Lines • ▼ Show 20 Linesdefine amdgpu_kernel void @v_test_umin3_ult_i16(i16 addrspace(1)* %out, i16 addrspace(1)* %aptr, i16 addrspace(1)* %bptr, i16 addrspace(1)* %cptr) #0 {
%icmp0 = icmp ult i16 %a, %b %icmp0 = icmp ult i16 %a, %b
%i0 = select i1 %icmp0, i16 %a, i16 %b %i0 = select i1 %icmp0, i16 %a, i16 %b
%icmp1 = icmp ult i16 %i0, %c %icmp1 = icmp ult i16 %i0, %c
%i1 = select i1 %icmp1, i16 %i0, i16 %c %i1 = select i1 %icmp1, i16 %i0, i16 %c
store i16 %i1, i16 addrspace(1)* %outgep store i16 %i1, i16 addrspace(1)* %outgep
ret void ret void
} }
; GCN-LABEL: {{^}}v_test_imin3_slt_i8:
arsenmUnsubmitted
Done
ReplyInline Actions

Max coverage would also be good, not just min

arsenm: Max coverage would also be good, not just min
sheredomAuthorUnsubmitted
Done
ReplyInline Actions

Good shout - added!

sheredom: Good shout - added!
; SI: v_min3_i32
; VI: v_min_i16
; VI: v_min_i16
; GFX9: v_min3_i16
define amdgpu_kernel void @v_test_imin3_slt_i8(i8 addrspace(1)* %out, i8 addrspace(1)* %aptr, i8 addrspace(1)* %bptr, i8 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i8, i8 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i8, i8 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i8, i8 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i8, i8 addrspace(1)* %out, i32 %tid
%a = load i8, i8 addrspace(1)* %gep0
%b = load i8, i8 addrspace(1)* %gep1
%c = load i8, i8 addrspace(1)* %gep2
%icmp0 = icmp slt i8 %a, %b
%i0 = select i1 %icmp0, i8 %a, i8 %b
%icmp1 = icmp slt i8 %i0, %c
%i1 = select i1 %icmp1, i8 %i0, i8 %c
store i8 %i1, i8 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_umin3_ult_i8:
; SI: v_min3_u32
; VI: v_min_u16
; VI: v_min_u16
; GFX9: v_min3_u16
define amdgpu_kernel void @v_test_umin3_ult_i8(i8 addrspace(1)* %out, i8 addrspace(1)* %aptr, i8 addrspace(1)* %bptr, i8 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i8, i8 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i8, i8 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i8, i8 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i8, i8 addrspace(1)* %out, i32 %tid
%a = load i8, i8 addrspace(1)* %gep0
%b = load i8, i8 addrspace(1)* %gep1
%c = load i8, i8 addrspace(1)* %gep2
%icmp0 = icmp ult i8 %a, %b
%i0 = select i1 %icmp0, i8 %a, i8 %b
%icmp1 = icmp ult i8 %i0, %c
%i1 = select i1 %icmp1, i8 %i0, i8 %c
store i8 %i1, i8 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_imin3_slt_i7:
; SI: v_min3_i32
; VI: v_min_i16
; VI: v_min_i16
; GFX9: v_min3_i16
define amdgpu_kernel void @v_test_imin3_slt_i7(i7 addrspace(1)* %out, i7 addrspace(1)* %aptr, i7 addrspace(1)* %bptr, i7 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i7, i7 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i7, i7 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i7, i7 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i7, i7 addrspace(1)* %out, i32 %tid
%a = load i7, i7 addrspace(1)* %gep0
%b = load i7, i7 addrspace(1)* %gep1
%c = load i7, i7 addrspace(1)* %gep2
%icmp0 = icmp slt i7 %a, %b
%i0 = select i1 %icmp0, i7 %a, i7 %b
%icmp1 = icmp slt i7 %i0, %c
%i1 = select i1 %icmp1, i7 %i0, i7 %c
store i7 %i1, i7 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_umin3_ult_i7:
; SI: v_min3_u32
; VI: v_min_u16
; VI: v_min_u16
; GFX9: v_min3_u16
define amdgpu_kernel void @v_test_umin3_ult_i7(i7 addrspace(1)* %out, i7 addrspace(1)* %aptr, i7 addrspace(1)* %bptr, i7 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i7, i7 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i7, i7 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i7, i7 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i7, i7 addrspace(1)* %out, i32 %tid
%a = load i7, i7 addrspace(1)* %gep0
%b = load i7, i7 addrspace(1)* %gep1
%c = load i7, i7 addrspace(1)* %gep2
%icmp0 = icmp ult i7 %a, %b
%i0 = select i1 %icmp0, i7 %a, i7 %b
%icmp1 = icmp ult i7 %i0, %c
%i1 = select i1 %icmp1, i7 %i0, i7 %c
store i7 %i1, i7 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_imin3_slt_i33:
; GCN-NOT: v_min3
define amdgpu_kernel void @v_test_imin3_slt_i33(i33 addrspace(1)* %out, i33 addrspace(1)* %aptr, i33 addrspace(1)* %bptr, i33 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i33, i33 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i33, i33 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i33, i33 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i33, i33 addrspace(1)* %out, i32 %tid
%a = load i33, i33 addrspace(1)* %gep0
%b = load i33, i33 addrspace(1)* %gep1
%c = load i33, i33 addrspace(1)* %gep2
%icmp0 = icmp slt i33 %a, %b
%i0 = select i1 %icmp0, i33 %a, i33 %b
%icmp1 = icmp slt i33 %i0, %c
%i1 = select i1 %icmp1, i33 %i0, i33 %c
store i33 %i1, i33 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_umin3_ult_i33:
; GCN-NOT: v_min3
define amdgpu_kernel void @v_test_umin3_ult_i33(i33 addrspace(1)* %out, i33 addrspace(1)* %aptr, i33 addrspace(1)* %bptr, i33 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i33, i33 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i33, i33 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i33, i33 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i33, i33 addrspace(1)* %out, i32 %tid
%a = load i33, i33 addrspace(1)* %gep0
%b = load i33, i33 addrspace(1)* %gep1
%c = load i33, i33 addrspace(1)* %gep2
%icmp0 = icmp ult i33 %a, %b
%i0 = select i1 %icmp0, i33 %a, i33 %b
%icmp1 = icmp ult i33 %i0, %c
%i1 = select i1 %icmp1, i33 %i0, i33 %c
store i33 %i1, i33 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_imin3_slt_i64:
; GCN-NOT: v_min3
define amdgpu_kernel void @v_test_imin3_slt_i64(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr, i64 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i64, i64 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i64, i64 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i64, i64 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i64, i64 addrspace(1)* %out, i32 %tid
%a = load i64, i64 addrspace(1)* %gep0
%b = load i64, i64 addrspace(1)* %gep1
%c = load i64, i64 addrspace(1)* %gep2
%icmp0 = icmp slt i64 %a, %b
%i0 = select i1 %icmp0, i64 %a, i64 %b
%icmp1 = icmp slt i64 %i0, %c
%i1 = select i1 %icmp1, i64 %i0, i64 %c
store i64 %i1, i64 addrspace(1)* %outgep
ret void
}
; GCN-LABEL: {{^}}v_test_umin3_ult_i64:
; GCN-NOT: v_min3
define amdgpu_kernel void @v_test_umin3_ult_i64(i64 addrspace(1)* %out, i64 addrspace(1)* %aptr, i64 addrspace(1)* %bptr, i64 addrspace(1)* %cptr) #0 {
%tid = call i32 @llvm.amdgcn.workitem.id.x()
%gep0 = getelementptr i64, i64 addrspace(1)* %aptr, i32 %tid
%gep1 = getelementptr i64, i64 addrspace(1)* %bptr, i32 %tid
%gep2 = getelementptr i64, i64 addrspace(1)* %cptr, i32 %tid
%outgep = getelementptr i64, i64 addrspace(1)* %out, i32 %tid
%a = load i64, i64 addrspace(1)* %gep0
%b = load i64, i64 addrspace(1)* %gep1
%c = load i64, i64 addrspace(1)* %gep2
%icmp0 = icmp ult i64 %a, %b
%i0 = select i1 %icmp0, i64 %a, i64 %b
%icmp1 = icmp ult i64 %i0, %c
%i1 = select i1 %icmp1, i64 %i0, i64 %c
store i64 %i1, i64 addrspace(1)* %outgep
ret void
}
declare i32 @llvm.amdgcn.workitem.id.x() #1 declare i32 @llvm.amdgcn.workitem.id.x() #1
attributes #0 = { nounwind } attributes #0 = { nounwind }
attributes #1 = { nounwind readnone speculatable } attributes #1 = { nounwind readnone speculatable }