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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

Mostly LGTM except some nits. The major one is avoiding the repeated lowering code for each of these cases



================
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) {
+
----------------
rtaylor wrote:
> 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)?
> 
> 
Yes


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5601
+
+    // handle BUFFER_LOAD_BYTE/UBYTE/SHORT/USHORT overloaded intrinsics
+    if (LoadVT.getScalarType() == MVT::i8 ||
----------------
Capitalize


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5608
+
+      SDVTList ResList = DAG.getVTList(MVT::i32, Ops[0].getValueType());
+      SDValue BufferLoad = DAG.getMemIntrinsicNode(Opc, DL, ResList,
----------------
You can just hardcoded this to MVT::Other


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5648
+
+    // handle buffer_load_ubyte/byte/ushort/short overloaded intrinsics
+    if (LoadVT.getScalarType() == MVT::i8 ||
----------------
Capitalize


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5653-5656
+      if (LoadVT.getScalarType() == MVT::i8)
+        Opc = AMDGPUISD::BUFFER_LOAD_UBYTE;
+      else
+        Opc = AMDGPUISD::BUFFER_LOAD_USHORT;
----------------
Ternary operator


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6339-6342
+      if (VDataType == MVT::i8)
+        Opc = AMDGPUISD::BUFFER_STORE_BYTE;
+      else
+        Opc = AMDGPUISD::BUFFER_STORE_SHORT;
----------------
Ternary operator


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6376
+
+    // handle buffer_store_byte/short overloaded intrinsics
+    EVT VDataType = VData.getValueType().getScalarType();
----------------
Capitalize


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:6382-6385
+      if (VDataType == MVT::i8)
+        Opc = AMDGPUISD::BUFFER_STORE_BYTE;
+      else
+        Opc = AMDGPUISD::BUFFER_STORE_SHORT;
----------------
Ternary operator


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7833
+  if (((Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_UBYTE &&
+      VTSign->getVT()  == MVT::i8) ||
+      (Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT && 
----------------
Extra space before ==


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7835
+      (Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT && 
+      VTSign->getVT()  == MVT::i16)) &&
+      Src.hasOneUse()) {
----------------
Extra space before ==


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42885