Page MenuHomePhabricator

AMDGPU/GlobalISel: Allow selection of scalar min/max
ClosedPublic

Authored by arsenm on Sep 15 2019, 4:26 PM.

Details

Summary

I believe all of the uniform/divergent pattern predicates are
redundant and can be removed. The uniformity bit already influences
the register class, and nothhing has broken when I've removed this and
others.

Diff Detail

Event Timeline

arsenm created this revision.Sep 15 2019, 4:26 PM
alex-t added inline comments.Sep 17 2019, 2:57 AM
lib/Target/AMDGPU/SOPInstructions.td
422

As you remove the "!isDiverent" predicate, I'd like to see the test that ensures that V_MIN/MAX etc are not selected for uniform smin/smax/umin/umax IR operations. The test should check not GlobalIsel but current instruction selection case.

arsenm marked an inline comment as done.Sep 17 2019, 7:01 AM
arsenm added inline comments.
lib/Target/AMDGPU/SOPInstructions.td
422

Those should already exist

alex-t added inline comments.Sep 18 2019, 6:51 AM
lib/Target/AMDGPU/SOPInstructions.td
422

Yes. We have min.ll, max.ll and sminmax.ll
All except min.ll have "-amdgpu-scalarize-global-loads=false" flag in RUN line.
That means that they will select v_min/max always.
This was temporary solution a while ago to avoid massive change in the test base.
We need to remove that flags at least from max.ll and sminmax.ll and update the test until proceeding with current change.

Formally, guarding both VALU and SALU variants with same predicate looks extensive.
I have made small test and tried your change - it works as expected.

Nevertheless, I'd like to have clean test base until we remove odd predicates.
So, my idea is to create the change that removes "-amdgpu-scalarize-global-loads=false" from min/max tests. After submitting it you can safely go ahead.
Obviously I will try to submit the test change asap.

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

The patterns are rejected for having custom predicate code. We don't need it as a standard pattern checks the register banks, which is what we really want. I think the cleanest compatibility solution would be to use divergence to guide register class selection, and somehow add the equivalent checks in the DAG patterns. Defining the equivalent GlobalISel pattern as always true would probably work, although leave a lot of clutter in the matching tables

I also have a question: why do you need to remove that predicates at all? Does it mean that to enable GlobalISel you'd need to remove all the divergent predicates stuff?

The patterns are rejected for having custom predicate code. We don't need it as a standard pattern checks the register banks, which is what we really want. I think the cleanest compatibility solution would be to use divergence to guide register class selection, and somehow add the equivalent checks in the DAG patterns. Defining the equivalent GlobalISel pattern as always true would probably work, although leave a lot of clutter in the matching tables

Sounds good but I'd like to keep existing stuff working until we get ready to switch to the new solution.

alex-t accepted this revision.Sep 19 2019, 11:58 PM

LGTM but make sure it passes recently updated lit tests.

This revision is now accepted and ready to land.Sep 19 2019, 11:58 PM
arsenm closed this revision.Sep 20 2019, 7:35 PM

r372450