This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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

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.
arsenm added a subscriber: arsenm.Dec 28 2016, 8:44 AM
arsenm added inline comments.
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 added inline comments.Dec 28 2016, 10:58 PM
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 added inline comments.Dec 28 2016, 11:09 PM
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 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.

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-.

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

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
This revision was automatically updated to reflect the committed changes.