This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Only map VOP operands to VGPRs
ClosedPublic

Authored by arsenm on Jan 13 2020, 9:03 AM.

Details

Summary

Previously this was allowing one SGPR in the first source
operand, which technically also avoided violating this for most
operations (but not for special cases reading vcc).

This trivially avoids violating the constant bus restriction. We do
need to write some new, smarter operand folds to pick the optimal SGPR
to use in some kind of post-isel fold, but that's purely an
optimization.

I was originally thinking we would pick which operands should be SGPRs
in RegBankSelect, but I think this isn't really manageable. There
would be additional complexity to handle every G_* instruction, and
then any nontrivial instruction patterns would need to know when to
avoid violating it, which is likely to be very error prone.

I think having all inputs being canonically copies to VGPRs will
simplify the operand folding logic. The current folding we do is
backwards, and only considers one operand at a time, relative to
operands it already has. It therefore poorly handles the case where
there is already a constant bus operand user. If all operands are
copies, it's somewhat simpler to consider all input operands at once
to choose the optimal constant bus user.

Since the failure mode for constant bus violations is now a verifier
error and not an selection failure, this moves towards a place where
we can turn on the fallback mode. The SGPR copy folding optimizations
can be left for later.

Diff Detail

Event Timeline

arsenm created this revision.Jan 13 2020, 9:03 AM
arsenm updated this revision to Diff 237710.Jan 13 2020, 9:19 AM

Fix a few special cases. I think a few more remain

arsenm updated this revision to Diff 237714.Jan 13 2020, 9:29 AM

Catch a few more special cases

nhaehnle accepted this revision.Jan 30 2020, 1:33 AM

This seems like a reasonable thing to do. Perhaps the future folding into SGPR operands could even fit into the GICombine infrastructure somehow, since it really is a combine-type operation. Anyway, this change LGTM.

This revision is now accepted and ready to land.Jan 30 2020, 1:33 AM