Page MenuHomePhabricator

GlobalISel: Fix RegBankSelect for REG_SEQUENCE
ClosedPublic

Authored by arsenm on Feb 27 2019, 8:32 PM.

Details

Reviewers
qcolombet
Summary

The AArch64 test was broken since the result register already had a
set register class, so this test was a no-op. The mapping verify call
would fail because the result size is not the same as the inputs like
in a copy or phi.

The AMDGPU testcases are half broken and introduce illegal VGPR->SGPR
copies which need much more work to handle correctly (same for phis),
but add them as a baseline.

Diff Detail

Event Timeline

arsenm created this revision.Feb 27 2019, 8:32 PM

Nice catch!

Comment below.

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
223

Is it possible that OperandsMapping[0] != nullptr at that point?

Essentially when we set OperandsMapping[0] we have either IsCopyLike == true and we will break out right after we set it or we set it outside of IsCopyLike == true then we will never get into that block.

arsenm marked an inline comment as done.Mar 14 2019, 6:58 AM
arsenm added inline comments.
lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
223

It's initialized to null by the SmallVector constructor, so this is the first time it's ever set. The IsCopyLike check is a loop invariant.

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually. I'm able to hack around reg_sequence in the backend now, but phi will require changing this and the iteration order to defer mapping phis until the operands are all already mapped

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Could you elaborate on that?

Essentially the logic here is: if at least one argument is set, we can use that reg bank for the destination, since this is just a glorified copy. I can see how this doesn't work for reg_sequence, but I don't get why phis are a problem.

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
223

Ok, where I was getting at is, this check is not necessary, since by construction OperandsMapping[0] is nullptr and after that we won't loop back here.

All of this logic for reg_sequence and phi is totally broken for AMDGPU, so I might just end up ripping all of this out eventually.

Could you elaborate on that?

Essentially the logic here is: if at least one argument is set, we can use that reg bank for the destination, since this is just a glorified copy. I can see how this doesn't work for reg_sequence, but I don't get why phis are a problem.

I was going to write a longer email when I got back to looking at seriously fixing this, but it's not copy-like when you can't copy from one of the source banks to the destination. If any input is a VGPR, the result must be a VGPR. Phi is complicated because the input banks might not be known yet, so you have to defer assigning it until they are known. You have to allow a partially mapped function, and keep trying to regbankselect until all of the phi operands are resolved. This is essentially what SIFixSGPRCopies has to do now.

arsenm updated this revision to Diff 190763.Mar 14 2019, 5:51 PM

Remove null check. I think this switches to picking the last operand's bank rather than the first, but no tests seem to care

qcolombet accepted this revision.Mar 21 2019, 11:19 AM
This revision is now accepted and ready to land.Mar 21 2019, 11:19 AM
arsenm closed this revision.Mar 21 2019, 1:44 PM

r356713