Skip to content

Conversation

lukel97
Copy link
Contributor

@lukel97 lukel97 commented Jan 10, 2024

This patch adds support for the -mrvv-vector-bits flag in the Flang driver, and
translates them to -mvscale-min/-mvscale-max.

The code was copied from the Clang toolchain (similarly to what was done for
AArch64's -msve-vector-bits flag) so it also supports the same
-mrvv-vector-bits=zvl mode.

Note that Flang doesn't yet define the __riscv_v_fixed_vlen macro, so the help
text has been updated to highlight that it's only defined for Clang.

This patch adds support for the -mrvv-vector-bits flag in the Flang driver, and
translates them to -mvscale-min/-mvscale-max.

The code was copied from the Clang toolchain (similarly to what was done for
AArch64's -msve-vector-bits flag) so it also supports the same
-mrvv-vector-bits=zvl mode.

Note that Flang doesn't yet define the __riscv_v_fixed_vlen macro, so the help
text has been updated to highlight that it's only defined for Clang.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' flang:driver flang Flang issues not falling into any other category labels Jan 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 10, 2024

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-flang-driver

@llvm/pr-subscribers-clang

Author: Luke Lau (lukel97)

Changes

This patch adds support for the -mrvv-vector-bits flag in the Flang driver, and
translates them to -mvscale-min/-mvscale-max.

The code was copied from the Clang toolchain (similarly to what was done for
AArch64's -msve-vector-bits flag) so it also supports the same
-mrvv-vector-bits=zvl mode.

Note that Flang doesn't yet define the __riscv_v_fixed_vlen macro, so the help
text has been updated to highlight that it's only defined for Clang.


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

6 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+4-2)
  • (modified) clang/lib/Driver/ToolChains/Flang.cpp (+51)
  • (modified) clang/lib/Driver/ToolChains/Flang.h (+7)
  • (modified) flang/test/Driver/driver-help-hidden.f90 (+2)
  • (modified) flang/test/Driver/driver-help.f90 (+2)
  • (added) flang/test/Driver/riscv-rvv-vector-bits.f90 (+51)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index bffdddc28aac60..4de738ef27ae8b 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4585,11 +4585,13 @@ let Flags = [TargetSpecific] in {
 def menable_experimental_extensions : Flag<["-"], "menable-experimental-extensions">, Group<m_Group>,
   HelpText<"Enable use of experimental RISC-V extensions.">;
 def mrvv_vector_bits_EQ : Joined<["-"], "mrvv-vector-bits=">, Group<m_Group>,
+  Visibility<[ClangOption, FlangOption]>,
   HelpText<"Specify the size in bits of an RVV vector register. Defaults to "
            "the vector length agnostic value of \"scalable\". Accepts power of "
            "2 values between 64 and 65536. Also accepts \"zvl\" "
-           "to use the value implied by -march/-mcpu. Value will be reflected "
-           "in __riscv_v_fixed_vlen preprocessor define (RISC-V only)">;
+           "to use the value implied by -march/-mcpu. On Clang, value will be "
+           "reflected in __riscv_v_fixed_vlen preprocessor define (RISC-V "
+           "only)">;
 
 def munaligned_access : Flag<["-"], "munaligned-access">, Group<m_Group>,
   HelpText<"Allow memory accesses to be unaligned (AArch32/AArch64/LoongArch/RISC-V only)">;
diff --git a/clang/lib/Driver/ToolChains/Flang.cpp b/clang/lib/Driver/ToolChains/Flang.cpp
index 41eaad3bbad0a3..ccb9f75e21e558 100644
--- a/clang/lib/Driver/ToolChains/Flang.cpp
+++ b/clang/lib/Driver/ToolChains/Flang.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "Flang.h"
+#include "Arch/RISCV.h"
 #include "CommonArgs.h"
 
 #include "clang/Basic/CodeGenOptions.h"
@@ -14,6 +15,8 @@
 #include "llvm/Frontend/Debug/Options.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/RISCVISAInfo.h"
+#include "llvm/TargetParser/RISCVTargetParser.h"
 
 #include <cassert>
 
@@ -203,6 +206,51 @@ void Flang::AddAArch64TargetArgs(const ArgList &Args,
   }
 }
 
+void Flang::AddRISCVTargetArgs(const ArgList &Args,
+                               ArgStringList &CmdArgs) const {
+  const llvm::Triple &Triple = getToolChain().getTriple();
+  // Handle -mrvv-vector-bits=<bits>
+  if (Arg *A = Args.getLastArg(options::OPT_mrvv_vector_bits_EQ)) {
+    StringRef Val = A->getValue();
+    const Driver &D = getToolChain().getDriver();
+
+    // Get minimum VLen from march.
+    unsigned MinVLen = 0;
+    StringRef Arch = riscv::getRISCVArch(Args, Triple);
+    auto ISAInfo = llvm::RISCVISAInfo::parseArchString(
+        Arch, /*EnableExperimentalExtensions*/ true);
+    // Ignore parsing error.
+    if (!errorToBool(ISAInfo.takeError()))
+      MinVLen = (*ISAInfo)->getMinVLen();
+
+    // If the value is "zvl", use MinVLen from march. Otherwise, try to parse
+    // as integer as long as we have a MinVLen.
+    unsigned Bits = 0;
+    if (Val.equals("zvl") && MinVLen >= llvm::RISCV::RVVBitsPerBlock) {
+      Bits = MinVLen;
+    } else if (!Val.getAsInteger(10, Bits)) {
+      // Only accept power of 2 values beteen RVVBitsPerBlock and 65536 that
+      // at least MinVLen.
+      if (Bits < MinVLen || Bits < llvm::RISCV::RVVBitsPerBlock ||
+          Bits > 65536 || !llvm::isPowerOf2_32(Bits))
+        Bits = 0;
+    }
+
+    // If we got a valid value try to use it.
+    if (Bits != 0) {
+      unsigned VScaleMin = Bits / llvm::RISCV::RVVBitsPerBlock;
+      CmdArgs.push_back(
+          Args.MakeArgString("-mvscale-max=" + llvm::Twine(VScaleMin)));
+      CmdArgs.push_back(
+          Args.MakeArgString("-mvscale-min=" + llvm::Twine(VScaleMin)));
+    } else if (!Val.equals("scalable")) {
+      // Handle the unsupported values passed to mrvv-vector-bits.
+      D.Diag(diag::err_drv_unsupported_option_argument)
+          << A->getSpelling() << Val;
+    }
+  }
+}
+
 static void addVSDefines(const ToolChain &TC, const ArgList &Args,
                          ArgStringList &CmdArgs) {
 
@@ -321,6 +369,9 @@ void Flang::addTargetOptions(const ArgList &Args,
     AddAMDGPUTargetArgs(Args, CmdArgs);
     break;
   case llvm::Triple::riscv64:
+    getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
+    AddRISCVTargetArgs(Args, CmdArgs);
+    break;
   case llvm::Triple::x86_64:
     getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
     break;
diff --git a/clang/lib/Driver/ToolChains/Flang.h b/clang/lib/Driver/ToolChains/Flang.h
index 8d35080e1c0c88..ec2e545a1d0b5c 100644
--- a/clang/lib/Driver/ToolChains/Flang.h
+++ b/clang/lib/Driver/ToolChains/Flang.h
@@ -70,6 +70,13 @@ class LLVM_LIBRARY_VISIBILITY Flang : public Tool {
   void AddAMDGPUTargetArgs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const;
 
+  /// Add specific options for RISC-V target.
+  ///
+  /// \param [in] Args The list of input driver arguments
+  /// \param [out] CmdArgs The list of output command arguments
+  void AddRISCVTargetArgs(const llvm::opt::ArgList &Args,
+                          llvm::opt::ArgStringList &CmdArgs) const;
+
   /// Extract offload options from the driver arguments and add them to
   /// the command arguments.
   /// \param [in] C The current compilation for the driver invocation
diff --git a/flang/test/Driver/driver-help-hidden.f90 b/flang/test/Driver/driver-help-hidden.f90
index 9a11a7a571ffcc..9f48e61eb74d7b 100644
--- a/flang/test/Driver/driver-help-hidden.f90
+++ b/flang/test/Driver/driver-help-hidden.f90
@@ -121,6 +121,8 @@
 ! CHECK-NEXT: -mllvm <value>          Additional arguments to forward to LLVM's option processing
 ! CHECK-NEXT: -mmlir <value>          Additional arguments to forward to MLIR's option processing
 ! CHECK-NEXT: -module-dir <dir>       Put MODULE files in <dir>
+! CHECK-NEXT: -mrvv-vector-bits=<value>
+! CHECK-NEXT:                          Specify the size in bits of an RVV vector register. Defaults to the vector length agnostic value of "scalable". Accepts power of 2 values between 64 and 65536. Also accepts "zvl" to use the value implied by -march/-mcpu. On Clang, value will be reflected in __riscv_v_fixed_vlen preprocessor define (RISC-V only)
 ! CHECK-NEXT: -msve-vector-bits=<value>
 ! CHECK-NEXT:                          Specify the size in bits of an SVE vector register. Defaults to the vector length agnostic value of "scalable". (AArch64 only)
 ! CHECK-NEXT: --no-offload-arch=<value>
diff --git a/flang/test/Driver/driver-help.f90 b/flang/test/Driver/driver-help.f90
index e0e74dc56f331e..4ab21f0f8904eb 100644
--- a/flang/test/Driver/driver-help.f90
+++ b/flang/test/Driver/driver-help.f90
@@ -107,6 +107,8 @@
 ! HELP-NEXT: -mllvm <value>          Additional arguments to forward to LLVM's option processing
 ! HELP-NEXT: -mmlir <value>          Additional arguments to forward to MLIR's option processing
 ! HELP-NEXT: -module-dir <dir>       Put MODULE files in <dir>
+! HELP-NEXT: -mrvv-vector-bits=<value>
+! HELP-NEXT:                          Specify the size in bits of an RVV vector register. Defaults to the vector length agnostic value of "scalable". Accepts power of 2 values between 64 and 65536. Also accepts "zvl" to use the value implied by -march/-mcpu. On Clang, value will be reflected in __riscv_v_fixed_vlen preprocessor define (RISC-V only)
 ! HELP-NEXT: -msve-vector-bits=<value>
 ! HELP-NEXT:                          Specify the size in bits of an SVE vector register. Defaults to the vector length agnostic value of "scalable". (AArch64 only)
 ! HELP-NEXT: --no-offload-arch=<value>
diff --git a/flang/test/Driver/riscv-rvv-vector-bits.f90 b/flang/test/Driver/riscv-rvv-vector-bits.f90
new file mode 100644
index 00000000000000..f57b6725891937
--- /dev/null
+++ b/flang/test/Driver/riscv-rvv-vector-bits.f90
@@ -0,0 +1,51 @@
+! -----------------------------------------------------------------------------
+! Tests for the -mrvv-vector-bits flag (taken from the clang test)
+! -----------------------------------------------------------------------------
+
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=128 2>&1 | FileCheck --check-prefix=CHECK-128 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=256 2>&1 | FileCheck --check-prefix=CHECK-256 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=512 2>&1 | FileCheck --check-prefix=CHECK-512 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=1024 2>&1 | FileCheck --check-prefix=CHECK-1024 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=2048 2>&1 | FileCheck --check-prefix=CHECK-2048 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=scalable 2>&1 | FileCheck --check-prefix=CHECK-SCALABLE %s
+
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gcv_zvl256b \
+! RUN:  -mrvv-vector-bits=zvl 2>&1 | FileCheck --check-prefix=CHECK-256 %s
+! RUN: %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gcv_zvl512b \
+! RUN:  -mrvv-vector-bits=zvl 2>&1 | FileCheck --check-prefix=CHECK-512 %s
+
+! CHECK-128: "-fc1"
+! CHECK-128-SAME: "-mvscale-max=2" "-mvscale-min=2"
+! CHECK-256: "-fc1"
+! CHECK-256-SAME: "-mvscale-max=4" "-mvscale-min=4"
+! CHECK-512: "-fc1"
+! CHECK-512-SAME: "-mvscale-max=8" "-mvscale-min=8"
+! CHECK-1024: "-fc1"
+! CHECK-1024-SAME: "-mvscale-max=16" "-mvscale-min=16"
+! CHECK-2048: "-fc1"
+! CHECK-2048-SAME: "-mvscale-max=32" "-mvscale-min=32"
+
+! CHECK-SCALABLE-NOT: "-mvscale-min=
+! CHECK-SCALABLE-NOT: "-mvscale-max=
+
+! Error out if an unsupported value is passed to -mrvv-vector-bits.
+! -----------------------------------------------------------------------------
+! RUN: not %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=16 2>&1 | FileCheck --check-prefix=CHECK-BAD-VALUE-ERROR %s
+! RUN: not %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=A 2>&1 | FileCheck --check-prefix=CHECK-BAD-VALUE-ERROR %s
+! RUN: not %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc_zve64x \
+! RUN:  -mrvv-vector-bits=131072 2>&1 | FileCheck --check-prefix=CHECK-BAD-VALUE-ERROR %s
+! RUN: not %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gc \
+! RUN:  -mrvv-vector-bits=zvl 2>&1 | FileCheck --check-prefix=CHECK-BAD-VALUE-ERROR %s
+! RUN: not %flang -c %s -### --target=riscv64-linux-gnu -march=rv64gcv \
+! RUN:  -mrvv-vector-bits=64 2>&1 | FileCheck --check-prefix=CHECK-BAD-VALUE-ERROR %s
+!
+! CHECK-BAD-VALUE-ERROR: error: unsupported argument '{{.*}}' to option '-mrvv-vector-bits='
+

Copy link
Contributor

@banach-space banach-space left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me, but somebody actually familiar with RISC-V should review as well.

Comment on lines 4588 to 4594
Visibility<[ClangOption, FlangOption]>,
HelpText<"Specify the size in bits of an RVV vector register. Defaults to "
"the vector length agnostic value of \"scalable\". Accepts power of "
"2 values between 64 and 65536. Also accepts \"zvl\" "
"to use the value implied by -march/-mcpu. Value will be reflected "
"in __riscv_v_fixed_vlen preprocessor define (RISC-V only)">;
"to use the value implied by -march/-mcpu. On Clang, value will be "
"reflected in __riscv_v_fixed_vlen preprocessor define (RISC-V "
"only)">;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very against using HelpText like this - it makes compiler "help" output (i.e. flang-new -help) unusable. Please split into HelpText and DocBrief and keep HelpText short and concise.

@banach-space
Copy link
Contributor

Thanks for addressing my comment!

The overall logic LGTM, but please wait for somebody to review the finer RISC-V details before landing this.

Copy link
Collaborator

@preames preames left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We should explore options for merging the option processing code for options supported by both clang and flang, but that's explicitly future work.

@lukel97 lukel97 merged commit 2c60d59 into llvm:main Jan 10, 2024
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
This patch adds support for the -mrvv-vector-bits flag in the Flang
driver, and
translates them to -mvscale-min/-mvscale-max.

The code was copied from the Clang toolchain (similarly to what was done
for
AArch64's -msve-vector-bits flag) so it also supports the same
-mrvv-vector-bits=zvl mode.

Note that Flang doesn't yet define the __riscv_v_fixed_vlen macro, so
the help
text has been updated to highlight that it's only defined for Clang.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category flang:driver flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants