[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:7832
+  if (Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_UBYTE ||
+      Src.getOpcode() == AMDGPUISD::BUFFER_LOAD_USHORT) {
+    DCI.DAG.viewGraph();
----------------
rtaylor wrote:
> arsenm wrote:
> > Should have a hasOneUse check
> You mean there should be a hasOneUse check on the SIGN_EXTEND_INREG right?
No, the buffer operation. If there are multiple uses you will end up creating multiple loads


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:7834
+    DCI.DAG.viewGraph();
+    auto *M = cast<MemSDNode>(Src);
+    SDValue Ops[] = {
----------------
rtaylor wrote:
> 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?
The number of bits in the sext_inreg may not match the load's from-8/16 bit source. You can test this with something like
%load = call llvm.amdgcn.buffer.load.i8()
%ext = zext i8 %load to i32
%shl = shl i32 %ext, 27
%shr = ashr i32 %shl, 27

There will need more shifts to clear the extra bits in the loaded value


Repository:
  rL LLVM

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

https://reviews.llvm.org/D42885