[clang][RISCV] Set vscale_range attribute based on VLEN

[reames]

Summary

Follow up on D135894, restructure code to work in terms of minimum and maximum VLEN coming from RISCVISAInfo.cpp. In the original review, I'd mentioned that MinVLEN was sometimes zero. This turns out to be a case of human error, combined with really bad (lack of) error reporting.

This patch adds appropriate tests for various vector extension combinations to show the mechanism works, but doesn't try to provide exhaustive coverage of the extension interactions. Presumably, that is already covered in existing tests elsewhere.

[craig.topper]

In the original review, I'd mentioned that MinVLEN was sometimes zero. That's still true, but apparently only happens if you specify multiple extensions in the -target_feature string. So, we can at least test "v" and "zve64x" on their own before I go figure out exactly what's going wrong regarding e.g. parsing "+v,+zvl512b".

I think you need to use `-target-feature "v" -target-feature "zvl512b".

[reames]

In D136106#3863147, @craig.topper wrote:

In the original review, I'd mentioned that MinVLEN was sometimes zero. That's still true, but apparently only happens if you specify multiple extensions in the -target_feature string. So, we can at least test "v" and "zve64x" on their own before I go figure out exactly what's going wrong regarding e.g. parsing "+v,+zvl512b".

I think you need to use `-target-feature "v" -target-feature "zvl512b".

So, this was part right. The other issue is I'd been using Zvl512b, and apparently this bit of parsing code is case sensitive. I'm going to look into improving the error reporting here.

[frasercrmck]

LGTM

[reames]

In D136106#3863202, @reames wrote:

In D136106#3863147, @craig.topper wrote:

In the original review, I'd mentioned that MinVLEN was sometimes zero. That's still true, but apparently only happens if you specify multiple extensions in the -target_feature string. So, we can at least test "v" and "zve64x" on their own before I go figure out exactly what's going wrong regarding e.g. parsing "+v,+zvl512b".

I think you need to use `-target-feature "v" -target-feature "zvl512b".

So, this was part right. The other issue is I'd been using Zvl512b, and apparently this bit of parsing code is case sensitive. I'm going to look into improving the error reporting here.

Replying back to myself here as I'm not quite sure where to put this...

So, it turns out target-feature is currently quite weird. At the *clang* level, we require individual target-feature options for each extensions. Using a compound "+v,+zvl512b" is not recognized by the clang code (i.e, the bit this patch was modifying). However, this raw string gets joined with the valid attributes, and set into the IR as "target-features"="+64bit,+v,+zvl512b" on the function. This string is then parsed by later consumers, and thus the extensions listed in the compound list *are* picked up during e.g. codegen. So, it turns out we both do, and don't, parse these compound target-feature strings.

I'm leaning in the direction of having clang just support the compound target-feature strings, but I haven't found the right place for that just yet.

clang/lib/Basic/Targets/RISCV.cpp

 }
 
 Optional<std::pair<unsigned, unsigned>>
 RISCVTargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
   if (LangOpts.VScaleMin || LangOpts.VScaleMax)
     return std::pair<unsigned, unsigned>(
         LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax);
 
-  if (hasFeature("v"))
-    // Minimum VLEN=128, Maximum VLEN=64k, and RISCV::RVVBitsPerBlock is 64.
-    return std::pair<unsigned, unsigned>(2, 1024);
+  if (unsigned MinVLen = ISAInfo->getMinVLen()) {
+    unsigned MaxVLen = ISAInfo->getMaxVLen();
+    // RISCV::RVVBitsPerBlock is 64.
+    return std::pair<unsigned, unsigned>(MinVLen/64, MaxVLen/64);
+  }
 
   return None;
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.
 bool RISCVTargetInfo::hasFeature(StringRef Feature) const {
   bool Is64Bit = getTriple().getArch() == llvm::Triple::riscv64;
   auto Result = llvm::StringSwitch<Optional<bool>>(Feature)

clang/test/CodeGen/riscv-vector-bits-vscale-range.c

 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -mvscale-max=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=2 -mvscale-max=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=4 -mvscale-max=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -mvscale-max=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=16 -mvscale-max=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=1 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=2 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=2 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=4 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=4 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=8 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=8 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=16 -S -emit-llvm -o - %s | FileCheck %s -D#VBITS=16 --check-prefix=CHECK-NOMAX
 // RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -mvscale-min=1 -mvscale-max=0 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-UNBOUNDED
-// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-V
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +v -target-feature +zvl512b -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVL
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64x -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64f -target-feature +f -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
+// RUN: %clang_cc1 -triple riscv64-none-linux-gnu -target-feature +zve64d -target-feature +f -target-feature +d -S -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-ZVE64
 
 // CHECK-LABEL: @func() #0
 // CHECK: attributes #0 = { {{.*}} vscale_range([[#VBITS]],[[#VBITS]]) {{.*}} }
 // CHECK-NOMAX: attributes #0 = { {{.*}} vscale_range([[#VBITS]],0) {{.*}} }
 // CHECK-UNBOUNDED: attributes #0 = { {{.*}} vscale_range(1,0) {{.*}} }
-// CHECK-NONE: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-V: attributes #0 = { {{.*}} vscale_range(2,1024) {{.*}} }
+// CHECK-ZVL: attributes #0 = { {{.*}} vscale_range(8,1024) {{.*}} }
+// CHECK-ZVE64: attributes #0 = { {{.*}} vscale_range(1,1024) {{.*}} }
 void func(void) {}

llvm/include/llvm/Support/RISCVISAInfo.h

public:
   void toFeatures(std::vector<StringRef> &Features,
                   std::function<StringRef(const Twine &)> StrAlloc) const;
 
   const OrderedExtensionMap &getExtensions() const { return Exts; };
 
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
+  unsigned getMaxVLen() const { return 65536; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
 
   bool hasExtension(StringRef Ext) const;
   std::string toString() const;
   std::vector<std::string> toFeatureVector() const;
   StringRef computeDefaultABI() const;