[PATCH] D96258: [AMDGPU] Introduce Strict WQM mode

[Piotr Sobczak via Phabricator via llvm-commits]

piotr added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:14
+/// The "strict" prefix denotes that the inactive lanes are enabled irrespective
+/// of the control flow.
 ///
----------------
critson wrote:
> I am not sure if this is really the clearest description of strict mode and control flow.
> Technically strict modes are disabled and reenabled through control flow.
> Both strict and non-strict modes "respect" control-flow, but with different semantics.
> Perhaps something like:
> The "strict" prefix indicates that inactive lanes do not take part in control flow, specifically an inactive lane enabled by a strict WQM/WWM will always be enabled irrespective of control flow decisions.
> Conversely in non-strict WQM inactive lanes may control flow decisions.
Thanks, I will incorporate your description in the code.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:203
              Register SavedWQM);
   void toStrictWWM(MachineBasicBlock &MBB, MachineBasicBlock::iterator Before,
+                   Register SaveOrig, char StrictStateNeeded);
----------------
critson wrote:
> I wonder if these should now just be called toStrictMode and fromStrictMode.
Agreed, that would be clearer - I will rename it.


================
Comment at: llvm/test/CodeGen/AMDGPU/wqm.ll:422
+
+; Check that Strict WQM writes aren't coalesced with non-strict writes, since
+; the Strict WQM write could clobber disabled channels in the non-strict one.
----------------
critson wrote:
> It's not clear to me how this test proves the "coalesced" part?
I patterned this test on the existing one for WWM (test_strictwwm4), which follows the same structure. It seems the comment is wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96258