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

[Philip Reames 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);
----------------
preames wrote:

@banach-space You seem to have taken my comment in an unexpected direction.  I was not arguing that RISCV was "supported" in any particular way.  I was making a purely stylistic point that intentionally breaking other targets seems undesirable.  In order for RISCV (or any target) to become supported, we have to do bringup work.  This patch, and the others for RISCV, are specifically around getting RISCV support to a point where it could be reasonable called supported.  We're not there yet; I don't think anyone would say otherwise.  

My argument for the current code structure is that supporting other future targets with reasonable default code structure will make the bringup efforts for them more straight forward.  As evidence for my argument, I used the fact that the prior code structure did exactly that for RISCV.  This is a stylistic concern, and I'll fully admit one we could reasonable disagree on.  I think you're wrong here, but if you care strongly about this particular stylistic question, I will defer to you because I just don't think it matters that much.  

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