[AMDGPU][True16] si-fold-operand selecting srcidx for v_mov_b16_t16_e64

[broxigarchen]

This is a follow up patch from #161764

This

1. revert an error in previous patch that the v_mov_b16_t16_e32 should still select idx 1 while only e64 should select idx 2 (this is cosmetic since this error won't be triggered, but still it's an error)
2. revert the utility function added in previous patch
3. added a src modifier check in isFoldCopyable check for v_mov_b16_t16_e64 (we never set the src mod so far, but it's a correct change)

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Brox Chen (broxigarchen)

Changes

This is a follow up patch from #161764

This

1. fix a error that the v_mov_b16_t16_e32 should still select idx 0
2. remove the utility function added in previous patch
3. added a src modifier check in isFoldCopyable check for v_mov_b16_t16_e64

---

Full diff: https://github.com/llvm/llvm-project/pull/162101.diff

3 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIFoldOperands.cpp (+1-1)
• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+2-27)
• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.h (-1)

diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index 90c828ba8dfab..5d34cdbbf20c5 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -932,7 +932,7 @@ static MachineOperand *lookUpCopyChain(const SIInstrInfo &TII,
   for (MachineInstr *SubDef = MRI.getVRegDef(SrcReg);
        SubDef && TII.isFoldableCopy(*SubDef);
        SubDef = MRI.getVRegDef(Sub->getReg())) {
-    unsigned SrcIdx = TII.getFoldableCopySrcIdx(*SubDef);
+    const int SrcIdx = MovOp == AMDGPU::V_MOV_B16_t16_e64 ? 2 : 1;
     MachineOperand &SrcOp = SubDef->getOperand(SrcIdx);
 
     if (SrcOp.isImm())
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 46757cf5fe90c..b411648af2255 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -3411,7 +3411,6 @@ void SIInstrInfo::insertSelect(MachineBasicBlock &MBB,
 bool SIInstrInfo::isFoldableCopy(const MachineInstr &MI) {
   switch (MI.getOpcode()) {
   case AMDGPU::V_MOV_B16_t16_e32:
-  case AMDGPU::V_MOV_B16_t16_e64:
   case AMDGPU::V_MOV_B32_e32:
   case AMDGPU::V_MOV_B32_e64:
   case AMDGPU::V_MOV_B64_PSEUDO:
@@ -3428,34 +3427,10 @@ bool SIInstrInfo::isFoldableCopy(const MachineInstr &MI) {
   case AMDGPU::AV_MOV_B32_IMM_PSEUDO:
   case AMDGPU::AV_MOV_B64_IMM_PSEUDO:
     return true;
-  default:
-    return false;
-  }
-}
-
-unsigned SIInstrInfo::getFoldableCopySrcIdx(const MachineInstr &MI) {
-  switch (MI.getOpcode()) {
-  case AMDGPU::V_MOV_B16_t16_e32:
   case AMDGPU::V_MOV_B16_t16_e64:
-    return 2;
-  case AMDGPU::V_MOV_B32_e32:
-  case AMDGPU::V_MOV_B32_e64:
-  case AMDGPU::V_MOV_B64_PSEUDO:
-  case AMDGPU::V_MOV_B64_e32:
-  case AMDGPU::V_MOV_B64_e64:
-  case AMDGPU::S_MOV_B32:
-  case AMDGPU::S_MOV_B64:
-  case AMDGPU::S_MOV_B64_IMM_PSEUDO:
-  case AMDGPU::COPY:
-  case AMDGPU::WWM_COPY:
-  case AMDGPU::V_ACCVGPR_WRITE_B32_e64:
-  case AMDGPU::V_ACCVGPR_READ_B32_e64:
-  case AMDGPU::V_ACCVGPR_MOV_B32:
-  case AMDGPU::AV_MOV_B32_IMM_PSEUDO:
-  case AMDGPU::AV_MOV_B64_IMM_PSEUDO:
-    return 1;
+    return !TII->hasAnyModifiersSet(MI);
   default:
-    llvm_unreachable("MI is not a foldable copy");
+    return false;
   }
 }
 
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.h b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
index cc59acf1ebd94..a21089f8e0fcc 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.h
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.h
@@ -417,7 +417,6 @@ class SIInstrInfo final : public AMDGPUGenInstrInfo {
                                   const MachineInstr &MIb) const override;
 
   static bool isFoldableCopy(const MachineInstr &MI);
-  static unsigned getFoldableCopySrcIdx(const MachineInstr &MI);
 
   void removeModOperands(MachineInstr &MI) const;
 

[arsenm]

Missing test changes

[broxigarchen]

Missing test changes

This patch is reverting some of the changes in #161764 so I think we don't need extra test here? The ir test added in the previous patch should cover it already

[broxigarchen]

ping!

[umangyadav]

Can we add some unit-tests to make sure this is working as expected ?

[broxigarchen]

test added in previous patch https://github.com/llvm/llvm-project/pull/161764/files#diff-fdc17816dfff25251b0acd60e624921d50180b5c2fd5e1c6673a7a57def7f5e4

[arsenm]

Yes, but according to point 1 in the description there was an error which is apparently not covered

[broxigarchen]

I did a quick check, actually at this stage we always create a e64 inst, and actually cannot create a e32 inst since the dest reg of v_mov_b16_t16_e32 is not allocatable. Thus the point 1 in the description is more cosmetic

[arsenm]

I think you can make hasAnyModifiers static instead

[broxigarchen]

This will leads to a chain of functions to be converted to static so I decided to do the opposite way

[jayfoad]

I agree with Matt, it's better to make more methods static if that works.

[broxigarchen]

Done! Just updated to the other way of converting more static function

[broxigarchen]

ping!