[PATCH] D76567: AMDGPU: Implement getMemcpyLoopLoweringType

[Jay Foad via Phabricator via llvm-commits]

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