Page MenuHomePhabricator

[AMDGPU] Enable SEXT divergence driven selection.
ClosedPublic

Authored by alex-t on Mar 16 2020, 6:04 AM.

Details

Summary

This change enable the divergence driven selection for the SEXT DAG opcode.

Diff Detail

Event Timeline

alex-t created this revision.Mar 16 2020, 6:04 AM

Test name is misleading, this should just go in the realgar sext test. Also I would expect this to already be well covered

alex-t added a comment.EditedMar 16 2020, 7:50 AM

Test name is misleading, this should just go in the realgar sext test. Also I would expect this to already be well covered

This is not covered properly. Most of the tests except those I have already updated assume that we select everything to scalars and rely on the latest SIFixSGPRCopies moveToVALU.
Also, their names are misleading as well:

sext-eliminate.ll - eliminate what?
sext-in-reg - it contains a couple of tests that are not scalar but they're testing some special conditions.

My test checks specifically the functionality added by the change - proper selection of i16 to i32 uniform/divergent and i16 to i64 uniform/divergent.
As for the name: does sext-divergence-driven-selection.ll looks better?
Also, I would prefer to have the tests checking divergence driven ISel separately until we remove moveToVALU path completely. Otherwise I always should expect someone accidentally removing selection predicates that generates no errors but silently go through the moveToVALU.

vpykhtin accepted this revision.Mar 17 2020, 6:47 AM

LGTM, assuming Matt's concern is addressed.

This revision is now accepted and ready to land.Mar 17 2020, 6:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 7:58 AM

Test name is misleading, this should just go in the realgar sext test. Also I would expect this to already be well covered

This is not covered properly. Most of the tests except those I have already updated assume that we select everything to scalars and rely on the latest SIFixSGPRCopies moveToVALU.
Also, their names are misleading as well:

sext-eliminate.ll - eliminate what?

This is an optimization test

sext-in-reg - it contains a couple of tests that are not scalar but they're testing some special conditions.

sext_inreg is a different opcode. You're looking for sign_extend.ll

My test checks specifically the functionality added by the change - proper selection of i16 to i32 uniform/divergent and i16 to i64 uniform/divergent.
As for the name: does sext-divergence-driven-selection.ll looks better?
Also, I would prefer to have the tests checking divergence driven ISel separately until we remove moveToVALU path completely. Otherwise I always should expect someone accidentally removing selection predicates that generates no errors but silently go through the moveToVALU.