- 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.
Details
- Reviewers
kzhuravl b-sumner rampitec arsenm - Commits
- rG6db1f5da4feb: [AMDGPU] Simplify memory legalizer (add missing virtual descructor)
rGa5a7c331e789: [AMDGPU] Simplify memory legalizer
rL334257: [AMDGPU] Simplify memory legalizer (add missing virtual descructor)
rL334241: [AMDGPU] Simplify memory legalizer
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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 |
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. |
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) |
test/CodeGen/AMDGPU/memory-legalizer-invalid-addrspace.mir | ||
---|---|---|
3 ↗ | (On Diff #149003) | OK I will give it a try. Thanks. |
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. |
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.