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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5605-5609
+      // set Opc based on data type
+      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:5651
+
+    // handle buffer_load_ubyte/byte/ushort/short overloaded intrinsics
+    if (LoadVT.getScalarType() == MVT::i8 ||
----------------
Capitalize


================
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) {
+
----------------
This looks identical to the other part, which is kind of surprising to me but this should be factored into something common


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:5655
+
+      // set Opc based on data type
+      if (LoadVT.getScalarType() == MVT::i8)
----------------
This comment can be removed


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


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7829
+SDValue SITargetLowering::performSignExtendInRegCombine(SDNode *N,
+                                                   DAGCombinerInfo &DCI) const {
+  SDValue Src = N->getOperand(0);
----------------
Formatting


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7832
+  if (Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_UBYTE ||
+      Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT) {
+    DCI.DAG.viewGraph();
----------------
Should have a hasOneUse check


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7833
+      Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT) {
+    DCI.DAG.viewGraph();
+    auto *M = cast<MemSDNode>(Src);
----------------
Leftover debugging


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7834
+    DCI.DAG.viewGraph();
+    auto *M = cast<MemSDNode>(Src);
+    SDValue Ops[] = {
----------------
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


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7839-7841
+      Src.getOperand(3),        // voffset -- will be set by setBufferOffsets
+      Src.getOperand(4),        // soffset -- will be set by setBufferOffsets
+      Src.getOperand(5),        // offset -- will be set by setBufferOffsets
----------------
"will be set by" part doesn't make sense here


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.load.ll:275
+  %tmp = call i8 @llvm.amdgcn.raw.buffer.load.i8(<4 x i32> %rsrc, i32 0, i32 0, i32 0)
+  %tmp2 = zext i8 %tmp to i32
+  %val = uitofp i32 %tmp2 to float
----------------
The base test case shouldn't have a extend of the use and directly use the value. You should also have one with an explicit zext


================
Comment at: test/CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.load.ll:307
+}
+
+;CHECK-LABEL: {{^}}raw_buffer_load_sshort:
----------------
Could use a testcase with a second non-extended use


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42885