This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: An extension to promote constant offset to the immediate
AbandonedPublic

Authored by cfang on Mar 26 2019, 11:16 AM.

Details

Reviewers
rampitec
arsenm
Summary

This is an extension to https://reviews.llvm.org/D55539 : promote constant offset to the immediate
to increase of the opportunity for loads.

Diff Detail

Event Timeline

cfang created this revision.Mar 26 2019, 11:16 AM
arsenm added inline comments.Mar 26 2019, 11:23 AM
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
1230–1231

Where are the _e32 versions coming from? I don't think you should be seeing these at this point.

You would need to verify the carry out is dead here. You should add a testcase where the vcc def of the add is used

test/CodeGen/AMDGPU/promote-constOffset-to-imm.ll
493

This shouldn't include a call

514–517

The test could be a lot simpler

cfang marked an inline comment as done.Mar 26 2019, 11:43 AM
cfang added inline comments.
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
1230–1231

These _e32 version was generated in "SI Fold Operands". If you think should should not happen, we may need to fix that first.

arsenm added inline comments.Mar 26 2019, 1:52 PM
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
1230–1231

This one is complicated. This still needs verification that the vcc use isn't needed

cfang marked 2 inline comments as done.Mar 29 2019, 3:03 PM
cfang added inline comments.
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
1230–1231

Do you mean the add AMDGPU::V_ADD_I32_e32 will actually been removed?

This still needs verification that the vcc use isn't needed

How to do this verification?

test/CodeGen/AMDGPU/promote-constOffset-to-imm.ll
493

Do you mean in general a LIT test shouldn't include a call, or just this test case? I saw a lot in the existing lit tests.

arsenm added inline comments.Jun 28 2019, 5:00 PM
lib/Target/AMDGPU/SILoadStoreOptimizer.cpp
1230–1231

.isDead() may be sufficient?

test/CodeGen/AMDGPU/promote-constOffset-to-imm.ll
493

Lit tests should generally not involve calls unless the call is specifically being tested. They add a lot of code and test complexity, and since the ABI is going to change, they may not be stable.

Is this still necessary?

cfang abandoned this revision.May 12 2021, 10:49 AM