AMDGPU: Remove redundant operand folding checks

[arsenm]

This was pre-filtering out a specific situation from being
added to the fold candidate list. The operand legality will
ultimately be checked with isOperandLegal before the fold is
performed, so I don't see the plus in pre-filtering this one
case.

[arsenm]

• AMDGPU: Handle folding vector splats of inline split f64 inline immediates #140878
• AMDGPU: Fix tracking subreg defs when folding through reg_sequence #140608
• AMDGPU: Add baseline tests for #139317 #140607
• AMDGPU: Remove redundant operand folding checks #140587 👈 (View in Graphite)
• AMDGPU: Delete seemingly dead s_fmaak_f32/s_fmamk_f32 folding code #140580
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

This was pre-filtering out a specific situation from being
added to the fold candidate list. The operand legality will
ultimately be checked with isOperandLegal before the fold is
performed, so I don't see the plus in pre-filtering this one
case.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIFoldOperands.cpp (-18)

diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index d94c2d8b03dff..3abc1be685e2e 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -778,24 +778,6 @@ bool SIFoldOperandsImpl::tryAddToFoldList(
       return true;
   }
 
-  // Check the case where we might introduce a second constant operand to a
-  // scalar instruction
-  if (TII->isSALU(MI->getOpcode())) {
-    const MCInstrDesc &InstDesc = MI->getDesc();
-    const MCOperandInfo &OpInfo = InstDesc.operands()[OpNo];
-
-    // Fine if the operand can be encoded as an inline constant
-    if (!OpToFold->isReg() && !TII->isInlineConstant(*OpToFold, OpInfo)) {
-      // Otherwise check for another constant
-      for (unsigned i = 0, e = InstDesc.getNumOperands(); i != e; ++i) {
-        auto &Op = MI->getOperand(i);
-        if (OpNo != i && !Op.isReg() &&
-            !TII->isInlineConstant(Op, InstDesc.operands()[i]))
-          return false;
-      }
-    }
-  }
-
   appendFoldCandidate(FoldList, MI, OpNo, OpToFold);
   return true;
 }

[Sisyph]

Your logic makes sense to me. Handing cases uniformly is good.

[arsenm]

Had to fix regression demonstrated in #140784 by avoiding folding a second frame index into an instruction that already uses one

[Sisyph]

LGTM

[arsenm]

Merge activity

• May 29, 5:28 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• May 29, 5:36 PM UTC: Graphite rebased this pull request as part of a merge.
• May 29, 5:38 PM UTC: @arsenm merged this pull request with Graphite.

[changpeng]

I am seeing a regression because of the removal of the above code:
For the following instructions:
%333:sreg_32 = S_MOV_B32 96
%334:sreg_32 = S_OR_B32 %stack.35.argument10.addr, %333:sreg_32, implicit-def dead $scc

After folding:
renamable $sgpr26 = S_OR_B32 896, 96, implicit-def dead $scc

@arsenm : can you explain why this pre-filtering is not necessary? Thanks.

[arsenm]

Because isOperandLegal is checked before the actual fold. This case is the special case with a frame index which we don't know the value of at this point which this avoids

[changpeng]

Because isOperandLegal is checked before the actual fold. This case is the special case with a frame index which we don't know the value of at this point which this avoids

So, should we put the pre-filtering code back, or fix something else?

[arsenm]

Fix something else. I'm pretty sure this exact case is already fixed in this patch