[PATCH] D76567: AMDGPU: Implement getMemcpyLoopLoweringType
Jay Foad via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 23 02:43:13 PDT 2020
foad added inline comments.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:325
+ unsigned MinAlign = std::min(SrcAlign, DstAlign);
+ if (MinAlign <= 1 || MinAlign >= 4) {
+ if ((SrcAddrSpace == AMDGPUAS::LOCAL_ADDRESS ||
----------------
I don't follow the reasoning here. When and why do we need to respect the alignment? Could you add comments?
The converse of `MinAlign <= 1 || MinAlign >= 4` is `MinAlign == 2` so maybe swap the `if` around:
```
if (MinAlign == 2)
...
else
...
```
?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:357
+ unsigned RemainingBytes, unsigned SrcAddrSpace, unsigned DestAddrSpace,
+ unsigned SrcAlign, unsigned DestAlign) const {
+ assert(RemainingBytes < 16);
----------------
Are you intentionally ignoring the alignment arguments?
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:363
+
+ while (RemainingBytes && RemainingBytes % 8 == 0) {
+ OpsOut.push_back(I64Ty);
----------------
Did you mean just `while (RemainingBytes >= 8)` here? If not the whole thing would be clearer structured as:
```
if (RemainingBytes % 8 == 0) {
... i64 loop ...
} else if (RemainingBytes % 4 == 0) {
... i32 loop ...
} else {
... i8 loop ...
}
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76567/new/
https://reviews.llvm.org/D76567
More information about the llvm-commits
mailing list