[flang-commits] [flang] [Flang][RISCV] Set vscale_range based off zvl*b (PR #77277)

[Andrzej Warzyński via flang-commits]

================
@@ -709,64 +709,81 @@ void CodeGenAction::lowerHLFIRToFIR() {
   }
 }
 
-// TODO: We should get this from TargetInfo. However, that depends on
-// too much of clang, so for now, replicate the functionality.
 static std::optional<std::pair<unsigned, unsigned>>
-getVScaleRange(CompilerInstance &ci) {
+getAArch64VScaleRange(CompilerInstance &ci) {
+  const auto &langOpts = ci.getInvocation().getLangOpts();
+
+  if (langOpts.VScaleMin || langOpts.VScaleMax)
+    return std::pair<unsigned, unsigned>(
+        langOpts.VScaleMin ? langOpts.VScaleMin : 1, langOpts.VScaleMax);
+
+  std::string featuresStr = ci.getTargetFeatures();
+  if (featuresStr.find("+sve") != std::string::npos)
+    return std::pair<unsigned, unsigned>(1, 16);
+
+  return std::nullopt;
+}
+
+static std::optional<std::pair<unsigned, unsigned>>
+getRISCVVScaleRange(CompilerInstance &ci) {
   const auto &langOpts = ci.getInvocation().getLangOpts();
   const auto targetOpts = ci.getInvocation().getTargetOpts();
   const llvm::Triple triple(targetOpts.triple);
 
-  if (triple.isAArch64()) {
-    if (langOpts.VScaleMin || langOpts.VScaleMax)
-      return std::pair<unsigned, unsigned>(
-          langOpts.VScaleMin ? langOpts.VScaleMin : 1, langOpts.VScaleMax);
-
-    std::string featuresStr = ci.getTargetFeatures();
-    if (featuresStr.find("+sve") != std::string::npos)
-      return std::pair<unsigned, unsigned>(1, 16);
-  } else if (triple.isRISCV()) {
-    auto parseResult = llvm::RISCVISAInfo::parseFeatures(
-        triple.isRISCV64() ? 64 : 32, targetOpts.featuresAsWritten);
-    if (!parseResult) {
-      std::string buffer;
-      llvm::raw_string_ostream outputErrMsg(buffer);
-      handleAllErrors(parseResult.takeError(), [&](llvm::StringError &errMsg) {
-        outputErrMsg << errMsg.getMessage();
-      });
-      ci.getDiagnostics().Report(clang::diag::err_invalid_feature_combination)
-          << outputErrMsg.str();
-      return std::nullopt;
-    }
+  auto parseResult = llvm::RISCVISAInfo::parseFeatures(
+      triple.isRISCV64() ? 64 : 32, targetOpts.featuresAsWritten);
+  if (!parseResult) {
+    std::string buffer;
+    llvm::raw_string_ostream outputErrMsg(buffer);
+    handleAllErrors(parseResult.takeError(), [&](llvm::StringError &errMsg) {
+      outputErrMsg << errMsg.getMessage();
+    });
+    ci.getDiagnostics().Report(clang::diag::err_invalid_feature_combination)
+        << outputErrMsg.str();
+    return std::nullopt;
+  }
 
-    llvm::RISCVISAInfo *const isaInfo = parseResult->get();
+  llvm::RISCVISAInfo *const isaInfo = parseResult->get();
 
-    // RISCV::RVVBitsPerBlock is 64.
-    unsigned vscaleMin = isaInfo->getMinVLen() / llvm::RISCV::RVVBitsPerBlock;
+  // RISCV::RVVBitsPerBlock is 64.
+  unsigned vscaleMin = isaInfo->getMinVLen() / llvm::RISCV::RVVBitsPerBlock;
 
-    if (langOpts.VScaleMin || langOpts.VScaleMax) {
-      // Treat Zvl*b as a lower bound on vscale.
-      vscaleMin = std::max(vscaleMin, langOpts.VScaleMin);
-      unsigned vscaleMax = langOpts.VScaleMax;
-      if (vscaleMax != 0 && vscaleMax < vscaleMin)
-        vscaleMax = vscaleMin;
-      return std::pair<unsigned, unsigned>(vscaleMin ? vscaleMin : 1,
-                                           vscaleMax);
-    }
+  if (langOpts.VScaleMin || langOpts.VScaleMax) {
+    // Treat Zvl*b as a lower bound on vscale.
+    vscaleMin = std::max(vscaleMin, langOpts.VScaleMin);
+    unsigned vscaleMax = langOpts.VScaleMax;
+    if (vscaleMax != 0 && vscaleMax < vscaleMin)
+      vscaleMax = vscaleMin;
+    return std::pair<unsigned, unsigned>(vscaleMin ? vscaleMin : 1, vscaleMax);
+  }
 
-    if (vscaleMin > 0) {
-      unsigned vscaleMax = isaInfo->getMaxVLen() / llvm::RISCV::RVVBitsPerBlock;
-      return std::make_pair(vscaleMin, vscaleMax);
-    }
-  } else {
-    if (langOpts.VScaleMin || langOpts.VScaleMax)
-      return std::pair<unsigned, unsigned>(
-          langOpts.VScaleMin ? langOpts.VScaleMin : 1, langOpts.VScaleMax);
+  if (vscaleMin > 0) {
+    unsigned vscaleMax = isaInfo->getMaxVLen() / llvm::RISCV::RVVBitsPerBlock;
+    return std::make_pair(vscaleMin, vscaleMax);
   }
 
   return std::nullopt;
 }
 
+// TODO: We should get this from TargetInfo. However, that depends on
+// too much of clang, so for now, replicate the functionality.
+static std::optional<std::pair<unsigned, unsigned>>
+getVScaleRange(CompilerInstance &ci) {
+  const auto &langOpts = ci.getInvocation().getLangOpts();
+  const llvm::Triple triple(ci.getInvocation().getTargetOpts().triple);
+
+  if (triple.isAArch64())
+    return getAArch64VScaleRange(ci);
+  if (triple.isRISCV())
+    return getRISCVVScaleRange(ci);
+
+  if (langOpts.VScaleMin || langOpts.VScaleMax)
+    return std::pair<unsigned, unsigned>(
+        langOpts.VScaleMin ? langOpts.VScaleMin : 1, langOpts.VScaleMax);
----------------
banach-space wrote:

> We had exactly this code structure previously, and it broke RISCV 

We are obviously very excited to see all the RISC-V contributions for Flang - it's great to see the increasing adoption! However, we need to be realistic - Flang is a very young project and the support for various architectures depends heavily on where the most contributions come from. A good proxy for which platforms are supported _well_ can be found in two places:
* https://github.com/llvm/llvm-project/blob/main/flang/docs/GettingStarted.md#supported-c-compilers
* https://lab.llvm.org/buildbot/#/builders (type "flang" in the search box).

RISC-V is not listed there - my assumption is that it reflects the fact that the interest is still rather limited.

Importantly, we can't really reason about the status of RISC-V support in Flang - we have no data for that. RISC-V buildbot for Flang would greatly help with patches like this one.

> Why should we bother to do something to actively break other (out of tree?) targets 

That's not the intent here and I don't know where you're getting that from. Instead, we are trying to make the level of the available support very clear. 

The corresponding options are only intended for AArch64 and RISC-V:

* https://github.com/llvm/llvm-project/blob/114e6d7ba02f090117f2cb1ffeb9027cf80f335b/clang/include/clang/Driver/Options.td#L4695-L4703

The driver should enforce that and that's what I propose here:

* https://github.com/llvm/llvm-project/pull/77905

If the preference is to indeed support other targets that implement scalable vectors (and require these options) then that should be tested. My preference is to simply limit the usage of `-mvscale-max` and `-mvscale-min` to AArch64+RISC-V. 

https://github.com/llvm/llvm-project/pull/77277