[RISCV][GISel] Attempt to simplify how we handle type legality for F and D extensions.

[topperc]

Add helper that creates a lambda similar to typeIs, but containing the predicate check for hasStdExtF and hasStdD. We can use this with legalIf and all to reduce the number of manual lambdas we need to write.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

Add helper that creates a lambda similar to typeIs, but containing the predicate check for hasStdExtF and hasStdD. We can use this with legalIf and all to reduce the number of manual lambdas we need to write.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp (+16-32)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
index c6654873118f98f..ffe35e39b9f3a91 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
@@ -23,6 +23,15 @@
 using namespace llvm;
 using namespace LegalityPredicates;
 
+static LegalityPredicate typeIsLegalScalarFP(unsigned TypeIdx,
+                                             const RISCVSubtarget &ST) {
+  return [=, &ST](const LegalityQuery &Query) {
+    return Query.Types[TypeIdx].isScalar() &&
+           ((ST.hasStdExtF() && Query.Types[TypeIdx].getSizeInBits() == 32) ||
+            (ST.hasStdExtD() && Query.Types[TypeIdx].getSizeInBits() == 64));
+  };
+}
+
 RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST) {
   const unsigned XLen = ST.getXLen();
   const LLT sXLen = LLT::scalar(XLen);
@@ -212,51 +221,26 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST) {
 
   getActionDefinitionsBuilder({G_FADD, G_FSUB, G_FMUL, G_FDIV, G_FMA, G_FNEG,
                                G_FABS, G_FSQRT, G_FMAXNUM, G_FMINNUM})
-      .legalIf([=, &ST](const LegalityQuery &Query) -> bool {
-        return (ST.hasStdExtF() && typeIs(0, s32)(Query)) ||
-               (ST.hasStdExtD() && typeIs(0, s64)(Query));
-      });
+      .legalIf(typeIsLegalScalarFP(0, ST));
 
   getActionDefinitionsBuilder(G_FPTRUNC).legalIf(
-      [=, &ST](const LegalityQuery &Query) -> bool {
-        return (ST.hasStdExtD() && typeIs(0, s32)(Query) &&
-                typeIs(1, s64)(Query));
-      });
+      all(typeIsLegalScalarFP(0, ST), typeIsLegalScalarFP(1, ST)));
   getActionDefinitionsBuilder(G_FPEXT).legalIf(
-      [=, &ST](const LegalityQuery &Query) -> bool {
-        return (ST.hasStdExtD() && typeIs(0, s64)(Query) &&
-                typeIs(1, s32)(Query));
-      });
+      all(typeIsLegalScalarFP(0, ST), typeIsLegalScalarFP(1, ST)));
 
   getActionDefinitionsBuilder(G_FCMP)
-      .legalIf([=, &ST](const LegalityQuery &Query) -> bool {
-        return typeIs(0, sXLen)(Query) &&
-               ((ST.hasStdExtF() && typeIs(1, s32)(Query)) ||
-                (ST.hasStdExtD() && typeIs(1, s64)(Query)));
-      })
+      .legalIf(all(typeIs(0, sXLen), typeIsLegalScalarFP(1, ST)))
       .clampScalar(0, sXLen, sXLen);
 
-  getActionDefinitionsBuilder(G_FCONSTANT)
-      .legalIf([=, &ST](const LegalityQuery &Query) -> bool {
-        return (ST.hasStdExtF() && typeIs(0, s32)(Query)) ||
-               (ST.hasStdExtD() && typeIs(0, s64)(Query));
-      });
+  getActionDefinitionsBuilder(G_FCONSTANT).legalIf(typeIsLegalScalarFP(0, ST));
 
   getActionDefinitionsBuilder({G_FPTOSI, G_FPTOUI})
-      .legalIf([=, &ST](const LegalityQuery &Query) -> bool {
-        return typeInSet(0, {s32, sXLen})(Query) &&
-               ((ST.hasStdExtF() && typeIs(1, s32)(Query)) ||
-                (ST.hasStdExtD() && typeIs(1, s64)(Query)));
-      })
+      .legalIf(all(typeInSet(0, {s32, sXLen}), typeIsLegalScalarFP(1, ST)))
       .widenScalarToNextPow2(0)
       .clampScalar(0, s32, sXLen);
 
   getActionDefinitionsBuilder({G_SITOFP, G_UITOFP})
-      .legalIf([=, &ST](const LegalityQuery &Query) -> bool {
-        return ((ST.hasStdExtF() && typeIs(0, s32)(Query)) ||
-                (ST.hasStdExtD() && typeIs(0, s64)(Query))) &&
-               typeInSet(1, {s32, sXLen})(Query);
-      })
+      .legalIf(all(typeIsLegalScalarFP(0, ST), typeInSet(1, {s32, sXLen})))
       .widenScalarToNextPow2(1)
       .clampScalar(1, s32, sXLen);
 

[topperc]

This is slighty different than the original logic. In RISC-V, hasStdExtD implies hasStdExtF and I assume a G_FPTRUNC where the source type isn't larger than the dest type is ill-formed. So I assume if the source type is legal and the dest type is legal the whole thing should be legal.

Same for G_FPEXT.

[michaelmaitland]

Simplifying this check has no performance gain, so maybe we should just keep as is? I expect things could get funky when we have to reason about the assumptions we made here when it is time to add Zfh and Zfhmin support.

[michaelmaitland]

When we add support for Zfh or Zfhmin in GISel, will you modify this function to check whether we hasZfh || hasZfhmin? What will happen if we only want to legalize an opcode when hasZfh but not when !hasZfh && hasZfhmin? Given these questions, is typeIsLegalScalarFP the correct name for this function?

[topperc]

This function will only get Zfh added to it. typeIsScalarFPArith? G_FTRUNC/G_FPEXT are the only ones touched by this patch that need Zfhmin.

[michaelmaitland]

I prefer typeIsScalarFPArith to typeIsLegalScalarFP since it doesn't have the word legal in it. I think the word legal is unnecessary since it will go into a legalIf which makes it redundant. I like ScalarFPArith better than ScalarFP since it will suggest that not all ScalarFP instructions use it.

[michaelmaitland]

LGTM