[PATCH] D22198: AMDGPU: Do not clobber SCC in SIWholeQuadMode

[Matt Arsenault via llvm-commits]

arsenm added a comment.

Is WQM needed for correctness or is it just an optimization?


================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:510-511
@@ +509,4 @@
+  const bool DefSCC = MI.definesRegister(AMDGPU::SCC);
+  const bool UseSCC = MI.readsRegister(AMDGPU::SCC);
+  const bool KillsSCC = MI.killsRegister(AMDGPU::SCC);
+
----------------
These can be combined with findRegisterUseOperandIdx

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:551
@@ +550,3 @@
+MachineBasicBlock::iterator
+SIWholeQuadMode::prepareInsert(MachineBasicBlock &MBB, bool SaveSCC) {
+  if (!EarlyInsertMI) {
----------------
This function is kind of complicated to special case track SCC liveness. Can you use LivePhysRegs or  check the LiveIntervals to simplify it (I think there was some issue with tracking physical registers with LIS last time I tried to do it, but I don't remember)

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:566-569
@@ +565,6 @@
+    MI = EarlyInsertMI;
+    Clean = true;
+  } else {
+    MI = LastInsertMI;
+    Clean = true;
+
----------------
The Clean = true can be hoisted to the initializer

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:572-573
@@ +571,4 @@
+    if (InsertSCCLive) {
+      for (MachineBasicBlock::iterator MII = LastInsertMI, MIE = MBB.end();
+           MII != MIE; ++MII) {
+        if (MII->readsRegister(AMDGPU::SCC)) {
----------------
range loop

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:574
@@ +573,3 @@
+           MII != MIE; ++MII) {
+        if (MII->readsRegister(AMDGPU::SCC)) {
+          Clean = false;
----------------
Maybe this should get the read operand and see if it is undef?

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:578-579
@@ +577,4 @@
+        }
+        if (MII->definesRegister(AMDGPU::SCC))
+          break;
+      }
----------------
Should check if it's dead?

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:597
@@ +596,3 @@
+    MachineInstr *Save =
+        BuildMI(MBB, MI, DebugLoc(), TII->get(AMDGPU::S_CSELECT_B32), SaveReg)
+            .addImm(1)
----------------
I think these should use MI's DebugLoc

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:605-606
@@ +604,4 @@
+
+    SI->insertMachineInstrInMaps(*Save);
+    SI->insertMachineInstrInMaps(*Restore);
+    LIS->createAndComputeVirtRegInterval(SaveReg);
----------------
Insert through LIS

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:609
@@ +608,3 @@
+
+    MI = Restore;
+  }
----------------
This can just return Restore

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:621
@@ -496,3 +620,3 @@
   if (SaveWQM) {
-    MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::S_AND_SAVEEXEC_B64),
+    MI = BuildMI(MBB, II, DebugLoc(), TII->get(AMDGPU::S_AND_SAVEEXEC_B64),
                  SaveWQM)
----------------
Should set DebugLoc

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:638
@@ -515,3 +637,3 @@
   if (SavedWQM) {
-    MI = BuildMI(MBB, Before, DebugLoc(), TII->get(AMDGPU::COPY), AMDGPU::EXEC)
+    MI = BuildMI(MBB, II, DebugLoc(), TII->get(AMDGPU::COPY), AMDGPU::EXEC)
              .addReg(SavedWQM);
----------------
Should set DebugLoc

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:675
@@ +674,3 @@
+  if (isEntry)
+    ++II; // skip the instruction that saves LiveMask
+
----------------
Capitalize

================
Comment at: test/CodeGen/AMDGPU/wqm.ll:423
@@ -422,1 +422,3 @@
 
+; Test awareness that s_wqm_b64 clobbers SCC.
+;
----------------
I don't see check lines for the cmp + select restore pattern here. Where is the scc def? This test also probably needs a comment

================
Comment at: test/CodeGen/AMDGPU/wqm.ll:429
@@ +428,3 @@
+; CHECK: s_cmp_
+; CHECK: s_cbranch_scc
+; CHECK: ; %if
----------------
CHECK-NEXT for s_cbranch_scc?


http://reviews.llvm.org/D22198