Page MenuHomePhabricator

DAG: Don't use ABI copies in some contexts
ClosedPublic

Authored by arsenm on Aug 8 2018, 9:56 AM.

Details

Summary

If an ABI-like value is used in a different block,
the type split used is not necessarily the same as
the call's ABI. The value is used through an intermediate
copy virtual registers from the other block. This
resulted in copies with inconsistent sizes later.

Fixes regressions since r338197 when AMDGPU started
splitting vector types for calls.

Diff Detail

Event Timeline

arsenm created this revision.Aug 8 2018, 9:56 AM
bjope added a comment.Aug 21 2018, 1:49 AM

(Not sure I'm able to review this, I don't know exactly how it works. I'll try to help out by asking some questions, maybe I will learn something to be confident enough to review it.)

The test cases you added only test the situation when the use is in another block. So what happens if the use is in the same block? Are we sure that we aren't breaking something for that scenario? (are modified functions only used for cross-block uses?)

I tried to find out where these ABI copy checks originates from. It partly comes from this https://reviews.llvm.org/D35765, although that patch only adds a test case that we do not crash. But the bulk of it seems to originate from https://reviews.llvm.org/D27845, which includes some test cases. So I guess we got some regression tests for the original purpose with these "is ABI copy" checks.

(Not sure I'm able to review this, I don't know exactly how it works. I'll try to help out by asking some questions, maybe I will learn something to be confident enough to review it.)

The test cases you added only test the situation when the use is in another block. So what happens if the use is in the same block? Are we sure that we aren't breaking something for that scenario? (are modified functions only used for cross-block uses?)

That's the normal case that most tests already us, so that works. The value is used directly without intermediate CopyToReg/CopyFromRegs

I tried to find out where these ABI copy checks originates from. It partly comes from this https://reviews.llvm.org/D35765, although that patch only adds a test case that we do not crash. But the bulk of it seems to originate from https://reviews.llvm.org/D27845, which includes some test cases. So I guess we got some regression tests for the original purpose with these "is ABI copy" checks.

The cases I added in r338197, r338416, r338418 and r338421 also use ABI copies

nhaehnle accepted this revision.Aug 28 2018, 3:00 AM

I'm not very familiar with this code either, but the change looks reasonable to me.

This revision is now accepted and ready to land.Aug 28 2018, 3:00 AM
arsenm closed this revision.Aug 29 2018, 10:50 PM

r341018