[PATCH] D35524: [AMDGPU] Add support for Whole Wavefront Mode

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Looks mostly good, but I do have a couple of comments.



================
Comment at: lib/Target/AMDGPU/SIFixWWMLiveness.cpp:26
+///
+/// The live intervals of %vgpr0 doesn't overlap with those of %vgpr1.
+/// Normally, we can safely allocate %vgpr0 and %vgpr1 in the same register,
----------------
*don't


================
Comment at: lib/Target/AMDGPU/SIFixWWMLiveness.cpp:100-101
+    AU.setPreservesCFG();
+    MachineFunctionPass::getAnalysisUsage(AU);
+    MachineFunctionPass::getAnalysisUsage(AU);
+  }
----------------
Duplicate function call.


================
Comment at: lib/Target/AMDGPU/SIFixWWMLiveness.cpp:118
+
+void SIFixWWMLiveness::addDefs(const MachineInstr &MI, SparseBitVector<> &regs)
+{
----------------
Upper case variable names.


================
Comment at: lib/Target/AMDGPU/SIFixWWMLiveness.cpp:168
+
+  bool modified = false;
+  for (unsigned Reg : LiveOut) {
----------------
Variable names are upper case in LLVM style.


================
Comment at: lib/Target/AMDGPU/SIFixWWMLiveness.cpp:183
+bool SIFixWWMLiveness::runOnMachineFunction(MachineFunction &MF) {
+  bool modified = false;
+
----------------
Variable names are upper case in LLVM style.


================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:139
 private:
+  CallingConv::ID callingConv;
   const SIInstrInfo *TII;
----------------
LLVM style is upper-case variable names.


================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:619-625
+  assert(SaveOrig);
+  MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::COPY), SaveOrig)
+           .addReg(AMDGPU::EXEC);
+  LIS->InsertMachineInstrInMaps(*MI);
+  MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_MOV_B64), AMDGPU::EXEC)
+           .addImm(-1);
+  LIS->InsertMachineInstrInMaps(*MI);
----------------
This could be an S_OR_SAVEEXEC_B64.


================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:784-785
             .addReg(Src);
+    if (MI->getOperand(0).isEarlyClobber())
+      Copy->getOperand(0).setIsEarlyClobber();
 
----------------
This should be simpler with MI->setDesc on the previous patch :)


================
Comment at: test/CodeGen/AMDGPU/wqm.ll:127-185
+; Check that we don't leave WWM on for computations that don't require WWM,
+; since that will lead clobbering things that aren't supposed to be clobbered
+; in cases like this.
+;
+;CHECK-LABEL: {{^}}test_wwm2:
+;CHECK: s_mov_b64 [[ORIG:s\[[0-9]+:[0-9]+\]]], exec
+;CHECK: s_mov_b64 exec, -1
----------------
Maybe I have jetlag blindness, but those two tests seem to be the same.

Also, there should be a test where the wwm computation does use some value from the predecessor block (e.g. use %hi as the offset in the load).


https://reviews.llvm.org/D35524