This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Simplify memory legalizer
ClosedPublic

Authored by t-tye on May 29 2018, 4:29 PM.

Details

Summary
  • Make code easier to maintain.
  • Avoid generating waitcnts for VMEM if the address sppace does not involve VMEM.
  • Add support to generate waitcnts for LDS and GDS memory.

Diff Detail

Repository
rL LLVM

Event Timeline

t-tye created this revision.May 29 2018, 4:29 PM
rampitec added inline comments.May 30 2018, 4:05 PM
lib/Target/AMDGPU/SIMemoryLegalizer.cpp
55 ↗(On Diff #149003)

typo: together

63 ↗(On Diff #149003)

typo: insert

88 ↗(On Diff #149003)

Why do we need this OTHER?

103 ↗(On Diff #149003)

Can you move this "MI" to the lane above?

212 ↗(On Diff #149003)

Please capitalize msg.

214 ↗(On Diff #149003)

typos.

219 ↗(On Diff #149003)

const?

222 ↗(On Diff #149003)

Please move AS to previous line. Line limit is 80 symbols.

223 ↗(On Diff #149003)

const

363 ↗(On Diff #149003)

I think rmw in the comment is misleading. It works with any atomic.

419 ↗(On Diff #149003)

Can you use colon after TODO? Some of search this way in sources. Same below.

463 ↗(On Diff #149003)

Why not llvm_unreachable? Do you really expect this to happen?

484 ↗(On Diff #149003)

IsNonTemporal &= !MMO->isNonTemporal();

But what if you have two memory operands, one non-temporal and other is not?

947 ↗(On Diff #149003)

typo: add

test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
1 ↗(On Diff #149003)

Please use -check-prefix=GCN

3 ↗(On Diff #149003)

Could you strip mir test from all IR part?

69 ↗(On Diff #149003)

All these attributes are also not needed in the test.

t-tye updated this revision to Diff 149241.May 31 2018, 12:38 AM

Update for @rampitec review comments.

t-tye marked 14 inline comments as done.May 31 2018, 12:50 AM
t-tye added inline comments.
lib/Target/AMDGPU/SIMemoryLegalizer.cpp
88 ↗(On Diff #149003)

It allows to determine if an instruction has only specific address spaces. To do that need to know it does not specify any other address spaces not explicitly modeled in atomics.

463 ↗(On Diff #149003)

I think these could happen for memory operations that have non-temporal.

484 ↗(On Diff #149003)

Only treat the instruction as non-temporal if all MMOs specify non-temporal. An MMO that is not non-temporal indicates want to cache.

test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
3 ↗(On Diff #149003)

The MMO references the IR to get the address space so I believe it is needed.

Should add some tests for this GDS support. Using a pure MIR test you can bypass the fact that we don't codegen it yet

test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
36–55 ↗(On Diff #149241)

You can definitely strip out all of the declarations

3 ↗(On Diff #149003)

The MMO should be able to support address spaces without an IR reference

t-tye marked an inline comment as done.May 31 2018, 1:56 AM
t-tye added inline comments.
test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
3 ↗(On Diff #149003)

Since the test is created from LLVM IR it seemed easier to leave the IR so the MMO is constructed with the correct address space. The alternative is to add an explicit address space to each MMO. This test follows the same approach of other existing tests, so if we want to change this we should also change those other tests.

arsenm added inline comments.May 31 2018, 2:00 AM
test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
3 ↗(On Diff #149003)

We have too many tests as-is that rely on the IR. Most of these are leftovers from before the MMO separately could track the address space, or before MIR supported some feature.

(dereferenceable invariant load 8 from i64 addrspace(4)* undef, addrspace 4)
This is all you need. The addrspace is a separate field, and I'm pretty sure if you use undef for all of the IR references you can drop the IR segment

t-tye added inline comments.May 31 2018, 2:05 AM
test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
3 ↗(On Diff #149003)

OK I will give it a try. Thanks.

t-tye updated this revision to Diff 149384.May 31 2018, 5:36 PM

Reduced size of mir tests.

t-tye marked 6 inline comments as done.May 31 2018, 5:38 PM
t-tye added inline comments.
test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir
3 ↗(On Diff #149003)

I have reduced the size of the mir tests. However, it was not possible to remove the llvm-ir as the mir parser requires each machine function to have a corresponding llvm-ir function.

t-tye marked an inline comment as done.May 31 2018, 5:52 PM

Should add some tests for this GDS support. Using a pure MIR test you can bypass the fact that we don't codegen it yet

Would it be ok to do that as a separate patch? Would like to add code selection patterns to handle lds and gds atomics and then use that to make an llvm ir test.

t-tye updated this revision to Diff 149577.Jun 1 2018, 3:40 PM

Further minimize MIR tests (thanks @rampitec ).

rampitec accepted this revision.Jun 1 2018, 3:48 PM

LGTM

This revision is now accepted and ready to land.Jun 1 2018, 3:48 PM
kzhuravl accepted this revision.Jun 1 2018, 8:34 PM

LGTM

t-tye updated this revision to Diff 150374.Jun 7 2018, 11:47 AM

Add MIR tests for local and region address spaces.

t-tye added a comment.Jun 7 2018, 11:49 AM

Should add some tests for this GDS support. Using a pure MIR test you can bypass the fact that we don't codegen it yet

Would it be ok to do that as a separate patch? Would like to add code selection patterns to handle lds and gds atomics and then use that to make an llvm ir test.

Added MIR tests for LDS and GDS.

This revision was automatically updated to reflect the committed changes.