[AMDGPU] Improve MIR pattern for FMinFMaxLegacy combine. NFC.

[jayfoad]

No description provided.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

Changes

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPUCombine.td (+3-2)
• (modified) llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp (+8-10)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUCombine.td b/llvm/lib/Target/AMDGPU/AMDGPUCombine.td
index 9218760538dc5d..9cd56f4cdb1f53 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUCombine.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUCombine.td
@@ -15,8 +15,9 @@ def fmin_fmax_legacy_matchdata : GIDefMatchData<"FMinFMaxLegacyInfo">;
 let Predicates = [HasFminFmaxLegacy] in
 def fcmp_select_to_fmin_fmax_legacy : GICombineRule<
   (defs root:$select, fmin_fmax_legacy_matchdata:$matchinfo),
-  (match (wip_match_opcode G_SELECT):$select,
-         [{ return matchFMinFMaxLegacy(*${select}, ${matchinfo}); }]),
+  (match (G_FCMP $cond, $pred, $lhs, $rhs):$fcmp,
+         (G_SELECT f32:$dst, $cond, $true, $false):$select,
+         [{ return matchFMinFMaxLegacy(*${select}, *${fcmp}, ${matchinfo}); }]),
   (apply [{ applySelectFCmpToFMinToFMaxLegacy(*${select}, ${matchinfo}); }])>;
 
 
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp b/llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
index 43ed14f350215d..fa8420b2f69782 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
@@ -70,7 +70,8 @@ class AMDGPUPostLegalizerCombinerImpl : public Combiner {
   };
 
   // TODO: Make sure fmin_legacy/fmax_legacy don't canonicalize
-  bool matchFMinFMaxLegacy(MachineInstr &MI, FMinFMaxLegacyInfo &Info) const;
+  bool matchFMinFMaxLegacy(MachineInstr &MI, MachineInstr &FCmp,
+                           FMinFMaxLegacyInfo &Info) const;
   void applySelectFCmpToFMinToFMaxLegacy(MachineInstr &MI,
                                          const FMinFMaxLegacyInfo &Info) const;
 
@@ -159,17 +160,14 @@ bool AMDGPUPostLegalizerCombinerImpl::tryCombineAll(MachineInstr &MI) const {
 }
 
 bool AMDGPUPostLegalizerCombinerImpl::matchFMinFMaxLegacy(
-    MachineInstr &MI, FMinFMaxLegacyInfo &Info) const {
-  // FIXME: Type predicate on pattern
-  if (MRI.getType(MI.getOperand(0).getReg()) != LLT::scalar(32))
-    return false;
-
-  Register Cond = MI.getOperand(1).getReg();
-  if (!MRI.hasOneNonDBGUse(Cond) ||
-      !mi_match(Cond, MRI,
-                m_GFCmp(m_Pred(Info.Pred), m_Reg(Info.LHS), m_Reg(Info.RHS))))
+    MachineInstr &MI, MachineInstr &FCmp, FMinFMaxLegacyInfo &Info) const {
+  if (!MRI.hasOneNonDBGUse(FCmp.getOperand(0).getReg()))
     return false;
 
+  Info.Pred =
+      static_cast<CmpInst::Predicate>(FCmp.getOperand(1).getPredicate());
+  Info.LHS = FCmp.getOperand(2).getReg();
+  Info.RHS = FCmp.getOperand(3).getReg();
   Register True = MI.getOperand(2).getReg();
   Register False = MI.getOperand(3).getReg();
 

[jayfoad]

Is there a way to do this check in the MIR pattern?

[tschuett]

No. Other uses could have sneaked in.

[jayfoad]

Sneaked in after the autogenerated matcher runs but before this match function is called? That doesn't sound right.

[tschuett]

You could:

GFCmp *Cmp = cast<GFCmp>(&FCmp);

[tschuett]

The G_FCMP could be used by other MIs.

[jayfoad]

The G_FCMP could be used by other MIs.

I know. I would like to write something in the pattern to check for this, instead of writing C++ to check for this.

[tschuett]

Open an issue?

[jayfoad]

Am I supposed to list $fcmp in the defs since I am passing it into my match function? Or doesn't that count as an "external" use of $fcmp?

[tschuett]

No. I prefer to only list the $root. defs is probably definitions and not def in SSA.

[tschuett]

You could hoist:

 if ((Info.LHS != True || Info.RHS != False) &&
      (Info.LHS != False || Info.RHS != True))
    return false;

into the pattern, but then you to write the pattern twice.