The pointer info for pseudo source for r600 is not correct when
alloca addr space is not 0, which causes invalid SDNode for r600---amdgiz.
This patch fixes that.
Differential D39670
[AMDGPU] Fix pointer info for pseudo source for r600 yaxunl on Nov 5 2017, 6:23 PM. Authored by
Details The pointer info for pseudo source for r600 is not correct when This patch fixes that.
Diff Detail
Event Timeline
Comment Actions We need to simplify the backend to support only one address space mapping. Supporting multiple address space mapping causes unnecessary complexity in the backend, e.g. intrinsic functions returning different pointer types based on triple, etc. Comment Actions Which intrinsic functions are you refering to? To me it seems easier to not touch r600 at all, but I guess I'm not as deep into the code as you. Can you give me some specific examples for how adding support to r600 for this alternative mapping simplifies the backend? Comment Actions For example, if we want to define a intrinsic function returning generic pointer, we have to define two versions. one for amdgiz environment, one for other environment, because they have different address space value for generic pointer. And we have to let the backend choose which version based on target environment. Also, because address space values depends on target triple, we can not define them as constants. Instead it has to be a structure returned by function getAMDGPUAS(triple) and use it. If we only supports one address space mapping, we can define it as an enum and use it. Comment Actions Sorry I forgot to mention: the plan is to let amdgcn and r600 only support the new address space mapping, then make it default and remove the old address space mapping. Comment Actions r600 doesn't support generic address space, so in that case, I would recommend only defining an intrinsic to be used by amdgcn. I understand the problem with non-constant address spaces, but I think the best solution here would be to try to make more of a separation between the amdgcn code and the r600 code in the backend rather than trying to change the address space mapping for r600. r600 should really be mostly read-only at this point. Comment Actions We discussed this internally and concluded that having a static address space mapping is more important. Most of issues are due to using dummy pointer info. Such places should be few in r600 code, therefore we will continue fixing these issues unless we found there need excessive efforts to do so. Comment Actions It would be nice to have these discussions on the mailing list so more people could participate, and it would be helpful for convincing people like me that this is the right approach, but as of right now I don't see why it is so important to have a static address space mapping that is the same for r600 and amdgcn. I think the better approach would be to share less code between the two subtargets such that it's possible for each to have their own static mapping. I think this kind of code separation is something should be done anyway independent of address space mapping work. Comment Actions I think the chance of R600 breaking with a different address space mapping is much higher. I think it is important to have the one static mapping that is consistent. Comment Actions LGTM with the switch fix. Also could just move the SI one to AMDGPUInstrInfo
Comment Actions 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. 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. Comment Actions
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 timeframe for migrating r600 to the new mapping?
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. Comment Actions Sure. I will keep Jan updated.
I plan to get it done ASAP. Hopefully in one month. I think the new address space mapping and old address space mapping share most of the code path in llvm/clang, so any regression for the old address space mapping is likely causes regression in the new address space mapping. On the other hand, since new address space mapping has non-zero alloca address space therefore some extra code path, regressions in new address space mapping may not cause regressions in old address space mapping.
|