This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Enable load-store-opt by default.
ClosedPublic

Authored by cfang on May 26 2016, 11:01 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

cfang updated this revision to Diff 58654.May 26 2016, 11:01 AM
cfang retitled this revision from to AMDGPU/SI: Enable load-store-opt by default..
cfang updated this object.
cfang added reviewers: arsenm, tstellarAMD, wdng.
cfang added subscribers: llvm-commits, arsenm.
arsenm accepted this revision.May 26 2016, 11:06 AM
arsenm edited edge metadata.

LGTM

test/CodeGen/AMDGPU/fmin3.ll
15–17 ↗(On Diff #58654)

Why did this test change as it doesn't use local memory?

This revision is now accepted and ready to land.May 26 2016, 11:06 AM
cfang added inline comments.May 26 2016, 11:22 AM
test/CodeGen/AMDGPU/fmin3.ll
15–17 ↗(On Diff #58654)

if (getOptLevel() > CodeGenOpt::None && ST.loadStoreOptEnabled()) {

  // Don't do this with no optimizations since it throws away debug info by
  // merging nonadjacent loads.

  // This should be run after scheduling, but before register allocation. It
  // also need extra copies to the address operand to be eliminated.
  insertPass(&MachineSchedulerID, &SILoadStoreOptimizerID);
  insertPass(&MachineSchedulerID, &RegisterCoalescerID);
}

I think the RegisterCoalescer Pass makes a difference. An ideal approach is to add the coalescer pass only when load-store-opt actually happens, but I think it is no harm here to have this additional coalescer pass.

arsenm added inline comments.May 26 2016, 11:23 AM
test/CodeGen/AMDGPU/fmin3.ll
15–17 ↗(On Diff #58654)

Oh, OK. That extra run is a workaround anyway. We should fix the pass to not depend on the scheduler and run in SSA form

This revision was automatically updated to reflect the committed changes.