[PATCH] D139852: [amdgpu] Lower CopyToReg into SGPR explicitly to avoid illegal vgpr to sgpr copy
Jon Chesterfield via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 12 12:04:18 PST 2022
JonChesterfield added a comment.
In D139852#3989451 <https://reviews.llvm.org/D139852#3989451>, @alex-t wrote:
> In D139852#3989299 <https://reviews.llvm.org/D139852#3989299>, @JonChesterfield wrote:
>
>> In D139852#3989251 <https://reviews.llvm.org/D139852#3989251>, @alex-t wrote:
>>
>>> Since we ensure all the VGPR to SGPR copies are uniform, we just need to V_READFIRSTLANE_B32 here.
>>
>> What ensures this copy is uniform?
>
> The divergence-driven ISel does.
> The call argument would be VGPR if it is divergent.
The call argument here isn't a vgpr - it's part of the calling convention for passing implicit parameters around, so it's explicitly a sgpr. We could have defined it to be a vgpr containing a uniform value instead, but as far as I can tell that would be strictly worse.
What we have at present is a constant node gets lowered to an instruction that leaves the result in a vgpr, and then miscompilation. We could indeed lower the constant to a vgpr and then notice that we've done that and insert a readfirstlane to fix it up, instead of erroring, that mostly means updating something downstream of ISel to know that a given vector register is uniform based on the instruction it came from.
I'm not totally convinced by our compiler emitting invalid code and then trying to fix it up later. This is a case where we can emit something which is correct up front, without divergence analysis or regressions. It's also associated with function calls which are quite high complexity as the baseline.
I think the right fix here is
- have globalisel model vector and scalar register files as separate things (if it doesn't already)
- teach regalloc about vector and scalar register files as most of the fixup pass seems to be trying to undo things regalloc did wrong
- delete sdag
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139852/new/
https://reviews.llvm.org/D139852
More information about the llvm-commits
mailing list