[PATCH] D39670: [AMDGPU] Fix pointer info for pseudo source for r600

Tom Stellard via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 9 09:21:02 PST 2017


tstellar added a subscriber: jvesely.
tstellar added a comment.

> ! In https://reviews.llvm.org/D39670#919711, @yaxunl wrote:
> 
> As Matt said, the risk of having two sets of address space mapping is high. Also I estimate the workload to switch r600 to the new addr space mapping is moderate. On the hand, the workload for separating r600 from amdgcn is high.

I disagree with this, but it's also possible I'm wrong.  If you are willing to do the work and deal with the fallout, then I'm OK with this change.  Just make sure  @jvesely is aware so he can help test/check for regressions.

One problem I have with this specific test is you are essentially removing a test for the current mapping and replacing it with a test for the new mapping, even though the new mapping isn't actually being used anywhere outside of LLVM.  What is your time from for migrating r600 to the new mapping?

> r600 and amdgcn share lots of passes. It is not practical to duplicate these passes and maintain them separately. Even if we were able to do that, maintaining two sets of static address space is error prone. On the other hand, let r600 switching to the new address space mapping takes only moderate efforts since it is only address space number change. We will fix the regressions arising from this change. After this change, we will get a more consistent and bug-free AMDGPU backend.

I started a branch to split these two apart and it wasn't as bad as I thought it would be.  I think long term this where we want to go with the backend.  Maybe at some point I'll get a chance to clean this branch up and send out the patches.


https://reviews.llvm.org/D39670





More information about the llvm-commits mailing list