Page MenuHomePhabricator

AMDGPU/SI: Don't promote alloca to vector for atomic load/store
ClosedPublic

Authored by cfang on Apr 25 2018, 2:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

cfang created this revision.Apr 25 2018, 2:33 PM
arsenm added inline comments.Apr 25 2018, 2:38 PM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
327–328 ↗(On Diff #144009)

You can use isSimple which checks volatile and atomic

test/CodeGen/AMDGPU/vector-alloca-atomic.ll
2 ↗(On Diff #144009)

You shouldn't need this

cfang updated this revision to Diff 144029.Apr 25 2018, 3:43 PM

Combine !isAtomic and !isVolatile checks as isSimple

cfang marked an inline comment as done.Apr 25 2018, 3:45 PM
cfang added inline comments.
test/CodeGen/AMDGPU/vector-alloca-atomic.ll
2 ↗(On Diff #144009)

If I remove the line "target datalayout = "A5"", I will get the following errors:

chfang@shanghai:~/bug/llvm/build/bin$ ./opt -S -mtriple=amdgcn---amdgiz -amdgpu-promote-alloca -sroa -instcombine < ../../test/CodeGen/AMDGPU/vector-alloca-atomic.ll | ./FileCheck -check-prefix=OPT ../../test/CodeGen/AMDGPU/vector-alloca-atomic.ll
Allocation instruction pointer not in the stack address space!

%alloca = alloca [3 x i32], addrspace(5)

Allocation instruction pointer not in the stack address space!

%alloca = alloca [3 x i32], addrspace(5)

Allocation instruction pointer not in the stack address space!

%alloca = alloca [3 x i32], addrspace(5)

./opt: -: error: input module is broken!
FileCheck error: '-' is empty.

cfang added a comment.Apr 26 2018, 1:48 PM

What do you think of the test?
What should we do if we don't add the following line?
target datalayout = "A5"

Thanks;

cfang updated this revision to Diff 144930.May 2 2018, 2:23 PM

Update the test:

  1. Remove the -amdgiz from the triple since it is no longer necessary;
  2. Add "-data-layout=A5" to explicitly specify the data layout for address space 5 for alloca
    • and to remove the "target:" line for the same data layout purpose.
arsenm accepted this revision.May 14 2018, 4:22 AM

LGTM with the check lines enhanced

test/CodeGen/AMDGPU/vector-alloca-atomic.ll
29 ↗(On Diff #144930)

These check lines should include the addrspace of the pointers used

50 ↗(On Diff #144930)

Ditto

This revision is now accepted and ready to land.May 14 2018, 4:22 AM
This revision was automatically updated to reflect the committed changes.