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

[Carl Ritson via Phabricator via llvm-commits]

critson 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.
 ///
----------------
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.


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


================
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.
----------------
It's not clear to me how this test proves the "coalesced" part?


================
Comment at: llvm/test/CodeGen/AMDGPU/wqm.ll:489
+;CHECK: v_add_f32_e32
+;CHECK: s_mov_b64 exec, [[ORIG2]]
+define amdgpu_ps float @test_strictwqm6_then() {
----------------
Might be worth clarify that this exit is in the %if branch.


================
Comment at: llvm/test/CodeGen/AMDGPU/wqm.ll:524
+;VI-CHECK: flat_load_dword
+;CHECK: s_mov_b64 exec, [[ORIG2]]
+define amdgpu_ps float @test_strictwqm6_loop() {
----------------
Same comment as previous test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96258