This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/SI: Move the local memory usage related checking after calling convention checking in PromoteAlloca
ClosedPublic

Authored by cfang on May 12 2017, 10:22 AM.

Details

Summary

Promoting Alloca to Vector and Promoting Alloca to LDS are two independent handling of Alloca and should not affect each other.
As a result, we should not give up promoting to vector if there is not enough LDS. This patch factors out the local memory usage
related checking out and replace it after the calling convention checking.

Diff Detail

Event Timeline

cfang created this revision.May 12 2017, 10:22 AM
arsenm added inline comments.May 12 2017, 10:26 AM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
605

There's a possible hazard with aliases of globals but I think that's been an existing problem

609–610

The user could be a constant expression which is transitively used by an instruction but I guess you're just moving this

704–705

This is being called for every single alloca in the function. This should probably be checked once earlier

test/CodeGen/AMDGPU/vector-alloca.ll
149–153

Should use the GCN instruction checks

cfang added inline comments.May 15 2017, 4:03 PM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
605

Will take a look and fix it in a separate patch if needed!

609–610

Will take a look and fix it in a separate patch if needed!

704–705

Done! Move the check before the loop, and use an argument to handleAlloca to carry the check result. Will update the diff late.

test/CodeGen/AMDGPU/vector-alloca.ll
149–153

what is GCN instruction check? Do you mean should use llc to compile to ISA and do checking?

cfang updated this revision to Diff 99157.May 16 2017, 9:20 AM
  1. Move the checking of available LDS outside the loop.
  2. Remove the ISA checking in the newly added test (I think the OPT checking is sufficient).
  1. the other two original issues Matt mentioned are to be investigated in a separate patch.
arsenm added inline comments.May 17 2017, 12:55 PM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
688–689

Why is this true? It failed

test/CodeGen/AMDGPU/vector-alloca.ll
149–153

You had EG checks before

cfang added inline comments.May 17 2017, 2:39 PM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
688–689

This debug message is a misleading! if tryPromoteAllocaToVector return true, it means alloca has already been vectoized, and we should return true here.

On the other hand, if tryPromoteAllocaToVector return false, we should continue to try to promote alloca to LDS. I will update the debug message!

test/CodeGen/AMDGPU/vector-alloca.ll
149–153

I know. That's what I copied and pasted from a previous test.
But the purpose of this new test is to verify that even though we have a local argument, we can still promote alloca to vector.
So, I think a OPT checking is enough! Do ISA checking is kind of redundant. You are right, if we do ISA checking, we should do GCN checking.

cfang updated this revision to Diff 99453.May 18 2017, 10:06 AM

Remove an incorrect debug message! Actually we do not need a debug message at the caller site for tryPromoteAllocaToVector because
in function tryPromoteAllocaToVector, debug message was dumped for every cases.

cfang added inline comments.May 18 2017, 10:09 AM
lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
688–689

So I completely remove this debug message here at the caller site because for every possible case in tryPromoteAllocaToVector,
a debug message was dumped.

test/CodeGen/AMDGPU/vector-alloca.ll
149–153

Let me know if you still went to check GCN for this new test, Thanks.

arsenm accepted this revision.May 23 2017, 9:28 AM

LGTM

This revision is now accepted and ready to land.May 23 2017, 9:28 AM
This revision was automatically updated to reflect the committed changes.