Page MenuHomePhabricator

AMDGPU: Adjust the chain for loads writing to the HI part of a register.
ClosedPublic

Authored by cfang on Jan 8 2019, 2:19 PM.

Details

Summary

For these loads that write to the HI part of a register, we should chain them to the op that writes to the LO part
of the register to maintain the appropriate order.

Diff Detail

Repository
rL LLVM

Event Timeline

cfang created this revision.Jan 8 2019, 2:19 PM

Can you add a test where low half does not produce a chain? An arithmetic operation and an undef.

lib/Target/AMDGPU/SIISelLowering.cpp
9027 ↗(On Diff #180737)

4 is not enough, it will not be a small vector. I think 16 is ok.

arsenm added a comment.Jan 8 2019, 2:57 PM

This problem isn't limited to private address space. This should have tests for every address space, and with cases using unrelated bases

cfang updated this revision to Diff 182114.Jan 16 2019, 12:15 PM

Update based on the comments from the reviewers:

  1. add test that the lower half op does not have a chain, so we don't adjust the chain;
  2. extend to every address space;
  3. add tests for unrelated bases;
  4. set the size of the small vector for Ops to 16.

Can you add a test where low half does not produce a chain? An arithmetic operation and an undef.

Tests added. Thanks.

This problem isn't limited to private address space. This should have tests for every address space, and with cases using unrelated bases

Done for other other address spaces. Thanks.

rampitec added inline comments.Jan 16 2019, 1:06 PM
test/CodeGen/AMDGPU/chain-hi-to-lo.ll
1 ↗(On Diff #182114)

Could you use -check-prefix=GCN ?

132 ↗(On Diff #182114)

addrspace(0) is not needed.

cfang marked 4 inline comments as done.Jan 16 2019, 1:23 PM
cfang added inline comments.
test/CodeGen/AMDGPU/chain-hi-to-lo.ll
1 ↗(On Diff #182114)

sure, thanks.

132 ↗(On Diff #182114)

will remove it. Thanks.

cfang updated this revision to Diff 182133.Jan 16 2019, 1:25 PM
cfang marked 2 inline comments as done.

Update based on reviewer's suggestions:

  1. -check-prefix=GCN
  2. addrspace(0) is not needed.
This revision is now accepted and ready to land.Jan 16 2019, 1:27 PM
This revision was automatically updated to reflect the committed changes.