[PATCH] D22195: AMDGPU: Move SIWholeQuadMode pass to after machine scheduling

[Nicolai Hähnle via llvm-commits]

nhaehnle added a comment.

Looks like some comments I wrote earlier were accidentally not sent...


================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:127
@@ -104,3 +126,3 @@
 
-  void printInfo();
+  void printInfo(MachineFunction &MF);
 
----------------
arsenm wrote:
> This should be const or moved to a static helper function. I think it will warn as unused in release builds
Changed to const. I see no unused warning yet, perhaps that only applies to static functions.

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:216
@@ -183,1 +215,3 @@
+
+      dbgs() << " " << left_justify(stateString(II.Needs), 5) << " " << MI;
     }
----------------
arsenm wrote:
> ditto
Done.

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:269-270
@@ +268,4 @@
+
+  assert(!II.Needs);
+  assert(Flag == StateWQM || Flag == StateExact);
+
----------------
arsenm wrote:
> You can combine these into one assert
I'd personally rather not, since they're logically about different things: the first one is about not changing an already marked instruction, the second about making sure that both flags are not set simultaneously.

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:535
@@ -419,3 +534,3 @@
 
-  if (!(BI.InNeeds & StateWQM))
+  if (!(BI.Needs.In & StateWQM))
     return;
----------------
arsenm wrote:
> Can you move the bit test into a helper function in the BI struct, same for everywhere else
The tests are all quite different, one would need many different helper functions. I think it would make the code harder to follow.

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:638
@@ +637,3 @@
+            .addReg(LiveMaskReg);
+    SI->replaceMachineInstrInMaps(*MI, *NewMI);
+
----------------
arsenm wrote:
> Why are you using SlotIndexes directly? I think in other places LIS->ReplaceMachineInstrInMaps is used
Done.

================
Comment at: lib/Target/AMDGPU/SIWholeQuadMode.cpp:688
@@ +687,3 @@
+    for (auto &BII : Blocks)
+      processBlock(const_cast<MachineBasicBlock &>(*BII.first), LiveMaskReg,
+                   BII.first == &*MF.begin());
----------------
arsenm wrote:
> You shouldn't need a const_cast (I thought I had a patch that fixed this before)
Thanks, that got lost in the rebase. Fixed now.


https://reviews.llvm.org/D22195