Page MenuHomePhabricator

AMDGPU: Support some GDS atomics
ClosedPublic

Authored by nhaehnle on Jun 17 2019, 11:50 AM.

Details

Summary

Original patch by Marek Olšák

Change-Id: Ia97d5d685a63a377d86e82942436d1fe6e429bab

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle created this revision.Jun 17 2019, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2019, 11:50 AM
arsenm added inline comments.Jun 17 2019, 11:53 AM
lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp
89–91 ↗(On Diff #205141)

I believe GDS can't be accessed through flat?

arsenm added inline comments.Jun 17 2019, 11:55 AM
test/CodeGen/AMDGPU/gds-atomic.ll
25 ↗(On Diff #205141)

Seems to be missing at least cmpxchg? Is this intended to handle all of the memory ops at once?

26 ↗(On Diff #205141)

Dead declaration

nhaehnle updated this revision to Diff 205158.Jun 17 2019, 12:55 PM
nhaehnle marked 2 inline comments as done.
  • region pointers cannot appear as flat pointers
  • expand the tests, handle the cmpxchg case
nhaehnle added inline comments.Jun 17 2019, 12:55 PM
lib/Target/AMDGPU/AMDGPUAnnotateKernelFeatures.cpp
89–91 ↗(On Diff #205141)

You're right.

arsenm added inline comments.Jun 18 2019, 4:37 PM
lib/Target/AMDGPU/SIISelLowering.cpp
5750–5751 ↗(On Diff #205158)

This is a bit magical

nhaehnle updated this revision to Diff 206212.Jun 24 2019, 7:15 AM
nhaehnle marked 2 inline comments as done.

Address review

lib/Target/AMDGPU/SIISelLowering.cpp
5750–5751 ↗(On Diff #205158)

Yes, but it's just hardware encoding. In any case, I'm pulling it into a separate patch since it's a logically separate change.

arsenm added inline comments.Jun 28 2019, 4:50 PM
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
137 ↗(On Diff #206212)

Probably should drop the global isel changes for now. I have a large patch replacing the load and store handling

lib/Target/AMDGPU/DSInstructions.td
604 ↗(On Diff #206212)

This can default to 0 instead of touching all the other patterns?

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
50 ↗(On Diff #206212)

Why is this not defaulting to 0?

nhaehnle marked 8 inline comments as done.Jul 1 2019, 9:16 AM
nhaehnle added inline comments.
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
137 ↗(On Diff #206212)

Sure, dropping this.

lib/Target/AMDGPU/DSInstructions.td
604 ↗(On Diff #206212)

Sure.

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
50 ↗(On Diff #206212)

Yeah, presumably it should. I'm changing it.

nhaehnle updated this revision to Diff 207347.Jul 1 2019, 9:16 AM
nhaehnle marked 3 inline comments as done.

Address review comments

arsenm accepted this revision.Jul 1 2019, 9:26 AM

LGTM

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
595–596 ↗(On Diff #207347)

Weird line wrap

This revision is now accepted and ready to land.Jul 1 2019, 9:26 AM
Closed by commit rL364814: AMDGPU: Support GDS atomics (authored by nha, committed by ). · Explain WhyJul 1 2019, 10:18 AM
This revision was automatically updated to reflect the committed changes.