Page MenuHomePhabricator

GlobalISel: Avoid producing Illegal copies in RegBankSelect
ClosedPublic

Authored by arsenm on Jun 11 2019, 1:21 PM.

Details

Reviewers
qcolombet
Summary

Avoid producing illegal register bank copies for reg_sequence and
phi. The default implementation assumes it is possible to pick any
operand's bank and use that for the result, introducing a copy for
operands with a different bank. This does not check for illegal
copies. It is not legal to introduce a VGPR->SGPR copy, so any VGPR
operand requires the result to be a VGPR.

The changes in getInstrMappingImpl aren't strictly necessary, since
AMDGPU now just bypasses this for reg_sequence/phi. This could be
replaced with an assert in case other targets run into this. It is
currently responsible for producing the error for unsatisfiable
copies, but this will be better served with a verifier check.

For phis, for now assume any undetermined operands must be
VGPRs. Eventually, this needs to be able to defer mapping these
operations. This also does not yet have a way to check for whether the
block is in a divergent region.

Diff Detail

Event Timeline

arsenm created this revision.Jun 11 2019, 1:21 PM
arsenm updated this revision to Diff 204159.Jun 11 2019, 1:43 PM

Remove unused variable

qcolombet accepted this revision.Jun 13 2019, 10:10 AM

LGTM.

Nitpicks below.

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
252

From LLVM standard: no brackets here.

418

Ditto

This revision is now accepted and ready to land.Jun 13 2019, 10:10 AM
arsenm marked an inline comment as done.Jun 13 2019, 11:42 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
252

I will die on the hill that there should be braces here. There are multiple lines after the if due to the comment. I don't see where it says this, and clang-format doesn't touch it

qcolombet added inline comments.Jun 13 2019, 12:43 PM
lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
252

I don't see anything in the standard mentioning the one-line-block thing now that you mention it. Which is strange because then it would be left to interpretation for case like:

if ()
  continue;

or

if () {
  continue;
}

whereas I thought we were very strict at using the former.

FWIW, clang will not remove the braces for the latter case either, so that's not a good indication of what is in the standard, unless the standard is: we don't care :).

Anyway, I don't really care, but I apply the rule for every one-statement-block. To me comments don't count as lines.

For instance, lib/Analysis/LazyCallGraph.cpp has one-statement block with comments (i.e., more than one line block in the end), but it still doesn't have braces.

arsenm closed this revision.Jun 14 2019, 8:20 AM
arsenm marked 2 inline comments as done.

r363410

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
252

There are examples of both, so I don't think there's a clear rule