This is an archive of the discontinued LLVM Phabricator instance.

Differential D28135

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

Authored by uabelho on Dec 28 2016, 5:17 AM.

Details

Reviewers
jvesely
• tstellarAMD
bogner
arsenm
Commits
rGb14afd452ddd: [SelectionDAG] Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486.
rL291441: [SelectionDAG] Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486.
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.

Diff Detail

Repository
rL LLVM

Event Timeline

uabelho updated this revision to Diff 82589.Dec 28 2016, 5:17 AM
uabelho retitled this revision from to SelectionDAG: Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486..
uabelho updated this object.
uabelho added reviewers: bogner, jvesely.
uabelho added a subscriber: llvm-commits.
Herald added a reviewer: • tstellarAMD. · View Herald TranscriptDec 28 2016, 5:17 AM
Herald added a subscriber: nhaehnle. · View Herald Transcript
arsenm added a subscriber: arsenm.Dec 28 2016, 8:44 AM
arsenm added inline comments.
test/CodeGen/AMDGPU/legalize-umax-bug.mir
1–2 ↗(On Diff #82589)

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?

RKSimon added a subscriber: RKSimon.Dec 28 2016, 2:43 PM
uabelho added inline comments.Dec 28 2016, 10:58 PM
test/CodeGen/AMDGPU/legalize-umax-bug.mir
1–2 ↗(On Diff #82589)

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 added inline comments.Dec 28 2016, 11:09 PM
test/CodeGen/AMDGPU/legalize-umax-bug.mir
1–2 ↗(On Diff #82589)

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 this revision to Diff 82651.Dec 28 2016, 11:44 PM
uabelho retitled this revision from SelectionDAG: Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486. to SelectionDAG: Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486..
uabelho updated this object.
uabelho edited edge metadata.

Updated the test case.

Herald edited edge metadata. · View Herald TranscriptDec 28 2016, 11:44 PM
Herald added a subscriber: wdng. · View Herald Transcript
jvesely added inline comments.Dec 29 2016, 6:51 AM
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 updated this revision to Diff 83015.Jan 4 2017, 12:58 AM
uabelho edited edge metadata.

Prefixed the test case name with r600-.

Herald edited edge metadata. · View Herald TranscriptJan 4 2017, 12:58 AM
uabelho added a subscriber: • test.Jan 4 2017, 1:01 AM

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 added a comment.Jan 4 2017, 8:41 AM

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 updated this revision to Diff 83196.Jan 4 2017, 11:48 PM
uabelho edited edge metadata.

Added CHECK in test case

Herald edited edge metadata. · View Herald TranscriptJan 4 2017, 11:48 PM
uabelho added a comment.Jan 4 2017, 11:49 PM

Anything else?

test/CodeGen/AMDGPU/r600-legalize-umax-bug.ll
4–5 ↗(On Diff #83015)

Done.

arsenm accepted this revision.Jan 5 2017, 9:50 AM
arsenm added a reviewer: arsenm.

LGTM

This revision is now accepted and ready to land.Jan 5 2017, 9:50 AM
Closed by commit rL291441: [SelectionDAG] Fix in legalization of UMAX/SMAX/UMIN/SMIN. Solves PR31486. (authored by bjope). · Explain WhyJan 9 2017, 4:14 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
CodeGen/
SelectionDAG/
4 lines
test/
CodeGen/
AMDGPU/
16 lines

Diff 83610

llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp

Show First 20 Lines • Show All 1,708 Lines • ▼ Show 20 Linesvoid DAGTypeLegalizer::ExpandIntRes_MINMAX(SDNode *N,
GetExpandedInteger(N->getOperand(0), LHSL, LHSH); GetExpandedInteger(N->getOperand(0), LHSL, LHSH);
GetExpandedInteger(N->getOperand(1), RHSL, RHSH); GetExpandedInteger(N->getOperand(1), RHSL, RHSH);
// Value types // Value types
EVT NVT = LHSL.getValueType(); EVT NVT = LHSL.getValueType();
EVT CCT = getSetCCResultType(NVT); EVT CCT = getSetCCResultType(NVT);
// Hi part is always the same op // 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' // We need to know whether to select Lo part that corresponds to 'winning'
// Hi part or if Hi parts are equal. // Hi part or if Hi parts are equal.
SDValue IsHiLeft = DAG.getSetCC(DL, CCT, LHSH, RHSH, CondC); SDValue IsHiLeft = DAG.getSetCC(DL, CCT, LHSH, RHSH, CondC);
SDValue IsHiEq = DAG.getSetCC(DL, CCT, LHSH, RHSH, ISD::SETEQ); SDValue IsHiEq = DAG.getSetCC(DL, CCT, LHSH, RHSH, ISD::SETEQ);
// Lo part corresponding to the 'winning' Hi part // Lo part corresponding to the 'winning' Hi part
SDValue LoCmp = DAG.getSelect(DL, NVT, IsHiLeft, LHSL, RHSL); SDValue LoCmp = DAG.getSelect(DL, NVT, IsHiLeft, LHSL, RHSL);
// Recursed Lo part if Hi parts are equal, this uses unsigned version // 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); Lo = DAG.getSelect(DL, NVT, IsHiEq, LoMinMax, LoCmp);
} }
void DAGTypeLegalizer::ExpandIntRes_ADDSUB(SDNode *N, void DAGTypeLegalizer::ExpandIntRes_ADDSUB(SDNode *N,
SDValue &Lo, SDValue &Hi) { SDValue &Lo, SDValue &Hi) {
SDLoc dl(N); SDLoc dl(N);
// Expand the subcomponents. // Expand the subcomponents.
▲ Show 20 Lines • Show All 1,716 Lines • Show Last 20 Lines

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

; RUN: llc -march=r600 -mcpu=cypress -start-after safe-stack %s -o - | FileCheck %s
; Don't crash
; CHECK: MAX_UINT
define void @test(i64 addrspace(1)* %out) {
bb:
store i64 2, i64 addrspace(1)* %out
%tmp = load i64, i64 addrspace(1)* %out
br label %jump
jump: ; preds = %bb
%tmp1 = icmp ugt i64 %tmp, 4
%umax = select i1 %tmp1, i64 %tmp, i64 4
store i64 %umax, i64 addrspace(1)* %out
ret void
}