[PATCH] D156669: AMDGPU: Handle llvm.stacksave and llvm.stackrestore

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/docs/ReleaseNotes.rst:77
 
+* Implemented `llvm.stacksave` and `llvm.stackrestore` intrinsics.
+
----------------
Pierre-vh wrote:
> Should this mention it's only for `p5`?
Not really, it's all that makes sense. the AMDGPUUsage note is enough


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:2582
+    CurDAG->getTargetConstant(Subtarget->getWavefrontSizeLog2(), SDLoc(N), MVT::i32);
+  CurDAG->SelectNodeTo(N, AMDGPU::S_LSHR_B32, N->getVTList(),
+                       {N->getOperand(0), Log2WaveSize});
----------------
Pierre-vh wrote:
> So this does: 
> ```
> swizzled SP = unswizzled SP / wavefront size
> ```
> But the docs say it does the opposite? (converts a swizzled address to unswizzled?)
> 
> My understanding of swizzling is pretty shallow, a bit more docs to help clarify this would be nice
I think the terminology is "swizzled" = "vgpr offset" and "unswizzled" = "sgpr offset" to a MUBUF instruction


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:3537
+  // stackrestore.
+  SDValue VectorAddress =
+      DAG.getNode(AMDGPUISD::WAVE_ADDRESS, SL, MVT::i32, CopyFromSP);
----------------
Pierre-vh wrote:
> How does changing the SP address to the swizzled SP help with edge cases?
> Also why do we need a new opcode for this?
An IR pointer is a vector value. A raw read of SP is the scalar value. The stack increments are really in wavesize chunks. If you do any addressing, or access of the pointer using the MUBUF voffset field you'll end up in the wrong place. The real usage is just write the direct value of a stacksave to a stackrestore call.

We need a new opcode to preserve the semantic intent. Otherwise the fold (whether into an addressing mode, or into the stackrestore call) will need to guess at the interpretation for a copy from a physical register. The call lowering has the same problem for matching the MUBUF addressing modes (GlobalISel uses the pseudo, the DAG gets away with inspecting the PseudoSourceValue for the underlying pointer)


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

https://reviews.llvm.org/D156669