[AMDGPU] Fix adding modifiers when creating v_cmpx instructions.

[tsymalla]

Summary

Revision https://reviews.llvm.org/D122332 added a pattern transformation
where v_cmpx instructions are introduced. However, the modifiers are
not correctly inherited from the original operands. The patch
adds the source modifiers, if they are exist, or sets them to 0.

[foad]

Looks OK but needs a lit test.

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

454–458

I'd slightly prefer a "getNamedImmOperandOrZero" which just returns the immediate value, instead of calling Builder.add.

456

Should probably reuse the index that you discarded on the line above, to avoid looking it up by name a second time.

[tsymalla]

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

454–458

The immediate value should only be added when the operand is found on the instruction, so the builder should only be called with 0 or the immediate value. When just returning the value, it would be required to have additional state which needs to be checked after calling the lambda to ensure a value needs to be added. This would delegate the responsibility back to the caller, but probably complicate everything.

I agree with renaming the object though.

[tsymalla]

Add lit test, address review comments.

[tsymalla]

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

454–458

I refactored this. As getNamedOperand returns nullptr when Idx == -1, the behavior should be the same.

[foad]

LGTM, thanks!

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

457

I think you might be able to simplify this to Builder.add(*Mod) but I'm not sure.

llvm/lib/Target/AMDGPU/SIOptimizeExecMasking.cpp

BuildMI(*SaveExecInstr.getParent(), InsertPosIt,
         .addReg(Exec);
   }
 
   // Omit dst as V_CMPX is implicitly writing to EXEC.
   // Add dummy src and clamp modifiers, if needed.
   auto Builder = BuildMI(*VCmp.getParent(), std::next(InsertPosIt),
                          VCmp.getDebugLoc(), TII->get(NewOpcode));
 
-  if (AMDGPU::getNamedOperandIdx(NewOpcode, AMDGPU::OpName::src0_modifiers) !=
-      -1)
-    Builder.addImm(0);
+  auto TryAddImmediateValueFromNamedOperand =
+      [&](unsigned OperandName) -> void {
+    if (auto *Mod = TII->getNamedOperand(VCmp, OperandName))
+      Builder.addImm(Mod->getImm());
+  };
 
+  TryAddImmediateValueFromNamedOperand(AMDGPU::OpName::src0_modifiers);
   Builder.add(*Src0);
 
-  if (AMDGPU::getNamedOperandIdx(NewOpcode, AMDGPU::OpName::src1_modifiers) !=
-      -1)
-    Builder.addImm(0);
-
+  TryAddImmediateValueFromNamedOperand(AMDGPU::OpName::src1_modifiers);
   Builder.add(*Src1);
 
-  if (AMDGPU::getNamedOperandIdx(NewOpcode, AMDGPU::OpName::clamp) != -1)
-    Builder.addImm(0);
+  TryAddImmediateValueFromNamedOperand(AMDGPU::OpName::clamp);
 
   return true;
 }
 
 bool SIOptimizeExecMasking::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(MF.getFunction()))
     return false;
 

llvm/test/CodeGen/AMDGPU/vcmp-saveexec-to-vcmpx.mir

-# RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -run-pass=si-optimize-exec-masking -verify-machineinstrs %s -o - | FileCheck -check-prefix=GCN %s
-# RUN: llc -march=amdgcn -mcpu=gfx1030 -mattr=-wavefrontsize32,+wavefrontsize64 -run-pass=si-optimize-exec-masking -verify-machineinstrs %s -o - | FileCheck -check-prefix=GCN %s
+# RUN: llc -march=amdgcn -mcpu=gfx1010 -mattr=-wavefrontsize32,+wavefrontsize64 -run-pass=si-optimize-exec-masking -verify-machineinstrs %s -o - | FileCheck --check-prefixes=GCN,GFX1010 %s
+# RUN: llc -march=amdgcn -mcpu=gfx1030 -mattr=-wavefrontsize32,+wavefrontsize64 -run-pass=si-optimize-exec-masking -verify-machineinstrs %s -o - | FileCheck --check-prefixes=GCN,GFX1030 %s
 
 ---
 
 # After the Optimize exec masking (post-RA) pass, there's a change of having v_cmpx instructions
 # being introduced whenever there's a sequence of v_cmp and s_and_saveexec instructions
 # which can be safely replaced in various cases.
 # However, it is not safe to do so when the generated code sequence would omit part of the EXEC mask
 # which could occur when a subset of EXEC is used as input operand in the v_cmp instruction.
 # The idea behind this test is to check if the subregisters are correctly handled here.
 
 # GCN-LABEL: name: vcmp_saveexec_to_mov_vcmpx_exec_subreg
 # GCN: V_CMP_GT_U32_e64
 # GCN: S_AND_SAVEEXEC_B64
 name: vcmp_saveexec_to_mov_vcmpx_exec_subreg
 tracksRegLiveness: true
 body: |
   bb.0:
     liveins: $vgpr0, $sgpr2
     renamable $sgpr0_sgpr1 = V_CMP_GT_U32_e64 $sgpr2, killed $vgpr0, implicit $exec
     $sgpr2_sgpr3 = COPY $exec, implicit-def $exec
     $sgpr2_sgpr3 = S_AND_B64 killed renamable $sgpr2_sgpr3, killed renamable $sgpr0_sgpr1, implicit-def dead $scc
     $exec = S_MOV_B64_term killed renamable $sgpr2_sgpr3
+...
+
+---
+# Check if the modifiers are preserved when generating the V_CMPX instruction.
+
+# GCN-LABEL: name: vcmp_saveexec_to_mov_vcmpx_check_abs
+# GFX1010: V_CMP_LT_F32_e64
+# GFX1010: S_AND_SAVEEXEC_B64
+# GFX1030: S_MOV_B64
+# GFX1030-NEXT: V_CMPX_LT_F32_nosdst_e64 0, 953267991, 2
+name: vcmp_saveexec_to_mov_vcmpx_check_abs
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $vgpr0
+    renamable $sgpr0_sgpr1 = V_CMP_LT_F32_e64 0, 953267991, 2, $vgpr0, 0, implicit $mode, implicit $exec
+    $sgpr2_sgpr3 = COPY $exec, implicit-def $exec
+    $sgpr2_sgpr3 = S_AND_B64 killed renamable $sgpr2_sgpr3, killed renamable $sgpr0_sgpr1, implicit-def dead $scc
+    $exec = S_MOV_B64_term killed renamable $sgpr2_sgpr3