[PATCH] D42885: [AMDGPU] intrintrics for byte/short load/store

[Ryan Taylor via Phabricator via llvm-commits]

rtaylor added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5652-5653
+    // handle buffer_load_ubyte/byte/ushort/short overloaded intrinsics
+    if (LoadVT.getScalarType() == MVT::i8 ||
+        LoadVT.getScalarType() == MVT::i16) {
+
----------------
arsenm wrote:
> This looks identical to the other part, which is kind of surprising to me but this should be factored into something common
Do you mean that there is simliar/same code in the different cases of buffer_load? (ie struct, raw, normal) And that these should be factored into something common (ie function call)?




================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7832
+  if (Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_UBYTE ||
+      Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT) {
+    DCI.DAG.viewGraph();
----------------
arsenm wrote:
> Should have a hasOneUse check
You mean there should be a hasOneUse check on the SIGN_EXTEND_INREG right?


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7834
+    DCI.DAG.viewGraph();
+    auto *M = cast<MemSDNode>(Src);
+    SDValue Ops[] = {
----------------
arsenm wrote:
> This is missing a check on the source type. If you want to be fancier, you can split out the remainder bits into a new sign extend but there probably isn't much reason to
Src is the BUFFER_LOAD_XXX. The only way this code is executed is if the Src is a BUFFER_LOAD_XXX. I'm not sure we need a redundant check here do we?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D42885/new/

https://reviews.llvm.org/D42885