Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

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
arsenm added inline comments.Mar 19 2019, 6:47 AM
lib/Target/AMDGPU/SIISelLowering.cpp
8137 ↗(On Diff #191282)

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 ↗(On Diff #191282)

Good idea, changed it to a whitelist!

sheredom marked an inline comment as done.Mar 19 2019, 7:25 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 ↗(On Diff #191298)

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

test/CodeGen/AMDGPU/min3.ll
161 ↗(On Diff #191298)

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 ↗(On Diff #191298)

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