[PATCH] D129073: [AMDGPU] Combine s_or_saveexec, s_xor instructions.

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

nhaehnle added a comment.

This looks pretty good already, but I do have a bunch of code quality comments inline.



================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:466
          J != JE; ++J) {
+
       if (SaveExecInst && J->readsRegister(Exec, TRI)) {
----------------
Why the empty line?


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:706-707
 
-  for (MachineBasicBlock &MBB : *MF) {
-    for (MachineInstr &MI : MBB) {
-      // Record relevant v_cmp / s_and_saveexec instruction pairs for
-      // replacement.
-      if (MI.getOpcode() != AndSaveExecOpcode)
-        continue;
+  // Record relevant v_cmp / s_and_saveexec instruction pairs for
+  // replacement.
+  if (MI.getOpcode() != AndSaveExecOpcode)
----------------
Confusing comment. Just remove it.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:711
 
-      if (MachineInstr *VCmp = findPossibleVCMPVCMPXOptimization(MI, Exec))
-        SaveExecVCmpMapping[&MI] = VCmp;
-    }
-  }
+  if (MachineInstr *VCmp = findPossibleVCMPVCMPXOptimization(MI))
+    SaveExecVCmpMapping[&MI] = VCmp;
----------------
Merge this function and the callee.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:715
 
+bool SIOptimizeExecMasking::optimizeVCmpxAndSaveexecSequences() {
+  bool Changed = false;
----------------
Inline this function into its caller.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:722-723
     if (optimizeSingleVCMPSaveExecSequence(*SaveExecInstr, *VCmpInstr, Exec)) {
       SaveExecInstr->eraseFromParent();
       VCmpInstr->eraseFromParent();
 
----------------
Move those into optmizeVCmpSaveExecSequence (also maybe rename the method while you're at it? The singular "sequence" already implies that it's a single sequence...)


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:751-754
+      // Peek at the next instruction and check if this is a relevant
+      // s_xor.
+      MachineInstr &PossibleXor = *MI.getNextNode();
+      if (PossibleXor.getOpcode() == XorOpcode) {
----------------
This is sort of backwards. When matching code for this kind of combine, the general pattern is to start from the definition of the final value you want to rewrite. In this case, that would be the S_XOR. You can see the approach taken in findPossibleVCMPVCMPXOptimization, using findInstrBackwards.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:765
+            OrXorPair.second = &PossibleXor;
+            OrXors.push_back(OrXorPair);
+          }
----------------
Remove OrXorPair and use `emplace_back` here


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:781-782
+                                              : AMDGPU::S_ANDN2_SAVEEXEC_B64;
+  MachineInstr *Or = nullptr;
+  MachineInstr *Xor = nullptr;
+  for (const auto &Pair : OrXors) {
----------------
Move into the inner scope.


================
Comment at: llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp:818-820
+      // While the or_saveexec sequence is most likely to
+      // appear at the end of the basic blocks, the v_cmp
+      // sequences can be spread over the whole basic block.
----------------
This isn't true. The v_cmp may potentially be far away, but the s_and_saveexec must be near the end of the block, so the entire loop should exit after the SearchWindow.

Also, and I know this isn't due to your change, but debug instructions should not be counted towards the search window. The goal is to avoid codegen changes caused by debug.

Also also, shouldn't we be able to exit this loop once we found the first write to EXEC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129073