[RISCV] Enable strict FP in clang as long as Zve* or V are not enabled.

[craig.topper]

Summary

As far as I know, scalar support for strict FP in the backend is
complete. Unfortunately, vector is missing.

This patch enables strict FP support as long as there's no chance
of any vector code. This isn't ideal, but it's a start.

Given the proximity to the LLVM 15 branch, I don't intend to commit this
until after that if this is approved.

[craig.topper]

clang/test/CodeGen/builtin_float_strictfp.c

3

I just picked a test to have some coverage that the strict FP is enabled. Most strict FP tests use -Xclang -fexperimental-strict-floating-point to avoid needing a specific target.

[craig.topper]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

934

Setting this to true disables some code in SelectionDAGISel.cpp that make ISD::STRICT_* nodes compile even if the target hasn't done the work.

[asb]

I don't feel very qualified on the current state of strict FP, but I've left a couple of minor comments inline. I'm happy to trust your knowledge the state of strictfp and the patch looks good to me otherwise.

clang/lib/Basic/Targets/RISCV.cpp

286

There's also code in RISCVISAInfo.cpp that does HasVector = Exts.count("zve32x") != 0. It's probably worth adding a helper (hasVInstructions?) that encapsulates this, and use it from both places.

clang/test/CodeGen/builtin_float_strictfp.c

3

I think it would be worth at least having coverage for the logic that disables strict FP if vector extensions are enabled.

[reames]

I'm not fluent on strict FP, so let me summarize my understanding. This is mostly so you can easily correct me if one my assumptions is wrong.

• Under strict FP, clang will emit constrained fp intrinsics instead of normal floating point ops.
• To my knowledge, clang will never emit an explicit vector constrained intrinsic.
• The vectorizers (LV, SLP) don't appear to have any handling for constrained FP intrinsics. If it did, I'd expect it to have to ask about legality of the widened operation - the same way it does for e.g. a scatter/gather.

So, my question is: why don't we support StrictFP when targeting a vector enabled platform? Don't we trivially support them by simply never using the vector forms?

clang/lib/Basic/Targets/RISCV.cpp

286

It's not clear to me why this condition is specific to embedded vector variants. Do we have strict FP with +V? Either you need to fix a comment here, or the condition. One or the other.

[craig.topper]

In D130311#3691029, @reames wrote:

I'm not fluent on strict FP, so let me summarize my understanding. This is mostly so you can easily correct me if one my assumptions is wrong.

• Under strict FP, clang will emit constrained fp intrinsics instead of normal floating point ops.
• To my knowledge, clang will never emit an explicit vector constrained intrinsic.

operator +, -, *, / etc. on attribute((vector_size)) types will generate vector constrained intrinsics.

• The vectorizers (LV, SLP) don't appear to have any handling for constrained FP intrinsics. If it did, I'd expect it to have to ask about legality of the widened operation - the same way it does for e.g. a scatter/gather.

So, my question is: why don't we support StrictFP when targeting a vector enabled platform? Don't we trivially support them by simply never using the vector forms?

clang/lib/Basic/Targets/RISCV.cpp

286

V implies Zve64d implies Zve64f implies Zve32f and Zve64x. Zve32f implies Zve32x. Zve32x is the root of the vector inheritance tree.

[reames]

In D130311#3691146, @craig.topper wrote:

In D130311#3691029, @reames wrote:

I'm not fluent on strict FP, so let me summarize my understanding. This is mostly so you can easily correct me if one my assumptions is wrong.

• Under strict FP, clang will emit constrained fp intrinsics instead of normal floating point ops.
• To my knowledge, clang will never emit an explicit vector constrained intrinsic.

operator +, -, *, / etc. on attribute((vector_size)) types will generate vector constrained intrinsics.

And probably also explicit intrinsic calls now that I'm thinking about it.

However, that doesn't really resolve my user interface concern. If I have purely scalar code, just adding +v to the extension list at the command line doesn't change whether strict FP is supported or not. This change would cause us to start reporting warnings which seems less than actionable to the user. It really seems like we need to be reporting warnings *when the explicit vector constructs are used*.

Worse than the warning bit, having strict FP scalar code stop being strict FP if you add +v seems... error prone.

(In case it's not clear, I can probably be convinced this is an imperfect step in the right general direction. I just want to make sure I fully understand the implications before giving an LGTM.)

clang/lib/Basic/Targets/RISCV.cpp

286

So, I went digging. I agree that our *implementation* treats V as implying Zve64d, but I can find anything in the *specification* to that effect. The feature set seems like it might be identical between the two, but I don't see anything in the spec which requires a +V implementation to claim support for Zve64d. Do you have particular wording in mind I'm missing?

(Regardless, the fact we assume this elsewhere means this is a non-blocking comment for this review. At the very least, this isn't introducing a new problem.)

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

286

We removed the implication for a brief period but Krste and Andrew disagreed. I believe this is now covered by the note at the end of https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#183-v-vector-extension-for-application-processors

"As is the case with other RISC-V extensions, it is valid to include overlapping extensions in the same ISA string. For example, RV64GCV and RV64GCV_Zve64f are both valid and equivalent ISA strings, as is RV64GCV_Zve64f_Zve32x_Zvl128b."

[reames]

clang/lib/Basic/Targets/RISCV.cpp

286

Er, yuck that's subtle. Not quite sure I'd read it the way you do, but your read is at least easily defensible. We can wait until someone has a concrete case where they aren't implied before figuring out if that case is disallowed per the spec. :)

[craig.topper]

clang/lib/Basic/Targets/RISCV.cpp

286

Maybe their biggest issue with the split we had was that we made them mutex and issued an error.

I'm going to add the wrapper that Alex suggested so that this is more centralized.

[craig.topper]

Add hasVInstructions to RISCVISAInfo

[kito-cheng]

clang/lib/Basic/Targets/RISCV.cpp

286

I think check zve32f would be better? zve32x means we have integer vector instruction but might not have any floating point vector instructions.

[asb]

llvm/lib/Support/RISCVISAInfo.cpp

232

Given the other sub-thread showed that hasExtennsion("zve32x") == hasVInstructions isn't completely obvious, perhaps worth a comment like "zve32x is the root of the V extension inheritance tree. i.e. enabling any of the vector extensions will implicitly enable zve32x."

clang/lib/Basic/Targets/RISCV.h

 //===--- RISCV.h - Declare RISCV target feature support ---------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
RISCVTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
     LongDoubleAlign = 128;
     LongDoubleFormat = &llvm::APFloat::IEEEquad();
     SuitableAlign = 128;
     WCharType = SignedInt;
     WIntType = UnsignedInt;
     HasRISCVVTypes = true;
     MCountName = "_mcount";
     HasFloat16 = true;
+    HasStrictFP = true;
   }
 
   bool setCPU(const std::string &Name) override {
     if (!isValidCPUName(Name))
       return false;
     CPU = Name;
     return true;
   }

clang/lib/Basic/Targets/RISCV.cpp

 //===--- RISCV.cpp - Implement RISCV target feature support ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (MinVLen) {
     Builder.defineMacro("__riscv_v_min_vlen", Twine(MinVLen));
     Builder.defineMacro("__riscv_v_elen", Twine(MaxELen));
     Builder.defineMacro("__riscv_v_elen_fp", Twine(MaxELenFp));
   }
 
   if (ISAInfo->hasExtension("c"))
     Builder.defineMacro("__riscv_compressed");
 
-  if (ISAInfo->hasExtension("zve32x"))
+  if (ISAInfo->hasVInstructions())
     Builder.defineMacro("__riscv_vector");
 }
 
 const Builtin::Info RISCVTargetInfo::BuiltinInfo[] = {
 #define BUILTIN(ID, TYPE, ATTRS)                                               \
   {#ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, nullptr},
 #define TARGET_BUILTIN(ID, TYPE, ATTRS, FEATURE)                               \
     {#ID, TYPE, ATTRS, nullptr, ALL_LANGUAGES, FEATURE},
if (!ParseResult) {
     return false;
   } else {
     ISAInfo = std::move(*ParseResult);
   }
 
   if (ABI.empty())
     ABI = ISAInfo->computeDefaultABI().str();
 
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasVInstructions())
+    HasStrictFP = false;
+
   return true;
 }
 
 bool RISCV32TargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::RISCV::checkCPUKind(llvm::RISCV::parseCPUKind(Name),
                                    /*Is64Bit=*/false);
 }
 

clang/test/CodeGen/builtin_float_strictfp.c

 // RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -triple x86_64-windows-pc -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,FP16
 // RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -triple ppc64-be -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,NOFP16
+// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -triple riscv64 -ffp-exception-behavior=maytrap -o - %s | FileCheck %s --check-prefixes=CHECK,FP16
 
 // test to ensure that these builtins don't do the variadic promotion of float->double.
 
 // Test that the constrained intrinsics are picking up the exception
 // metadata from the AST instead of the global default from the command line.
 
 #pragma float_control(except, on)
 

llvm/include/llvm/Support/RISCVISAInfo.h

 //===-- RISCVISAInfo.h - RISCV ISA Information ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
public:
 
   const OrderedExtensionMap &getExtensions() const { return Exts; };
 
   unsigned getXLen() const { return XLen; };
   unsigned getFLen() const { return FLen; };
   unsigned getMinVLen() const { return MinVLen; }
   unsigned getMaxELen() const { return MaxELen; }
   unsigned getMaxELenFp() const { return MaxELenFp; }
+  bool hasVInstructions() const;
 
   bool hasExtension(StringRef Ext) const;
   std::string toString() const;
   std::vector<std::string> toFeatureVector() const;
   StringRef computeDefaultABI() const;
 
   static bool isSupportedExtensionFeature(StringRef Ext);
   static bool isSupportedExtension(StringRef Ext);

llvm/lib/Support/RISCVISAInfo.cpp

 //===-- RISCVISAInfo.cpp - RISCV Arch String Parser --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
bool RISCVISAInfo::isSupportedExtension(StringRef Ext, unsigned MajorVersion,
   auto FindByNameAndVersion = [=](const RISCVSupportedExtension &ExtInfo) {
     return ExtInfo.Name == Ext && (MajorVersion == ExtInfo.Version.Major) &&
            (MinorVersion == ExtInfo.Version.Minor);
   };
   return llvm::any_of(SupportedExtensions, FindByNameAndVersion) ||
          llvm::any_of(SupportedExperimentalExtensions, FindByNameAndVersion);
 }
 
+bool RISCVISAInfo::hasVInstructions() const {
+  return hasExtension("zve32x");
+}
+
 bool RISCVISAInfo::hasExtension(StringRef Ext) const {
   stripExperimentalPrefix(Ext);
 
   if (!isSupportedExtension(Ext))
     return false;
 
   return Exts.count(Ext.str()) != 0;
 }

 
 Error RISCVISAInfo::checkDependency() {
   bool IsRv32 = XLen == 32;
   bool HasE = Exts.count("e") != 0;
   bool HasD = Exts.count("d") != 0;
   bool HasF = Exts.count("f") != 0;
   bool HasZfinx = Exts.count("zfinx") != 0;
   bool HasZdinx = Exts.count("zdinx") != 0;
-  bool HasVector = Exts.count("zve32x") != 0;
+  bool HasVector = hasVInstructions();
   bool HasZve32f = Exts.count("zve32f") != 0;
   bool HasZve64d = Exts.count("zve64d") != 0;
   bool HasZvl = MinVLen != 0;
 
   if (HasE && !IsRv32)
     return createStringError(
         errc::invalid_argument,
         "standard user-level extension 'e' requires 'rv32'");

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

 //===-- RISCVISelLowering.cpp - RISCV DAG Lowering Implementation  --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
if (Subtarget.useRVVForFixedLengthVectors()) {
         setOperationAction(ISD::BITCAST, MVT::f16, Custom);
       if (Subtarget.hasStdExtF())
         setOperationAction(ISD::BITCAST, MVT::f32, Custom);
       if (Subtarget.hasStdExtD())
         setOperationAction(ISD::BITCAST, MVT::f64, Custom);
     }
   }
 
+  // FIXME: Strict FP support is incomplete for vectors.
+  if (!Subtarget.hasVInstructions())
+    IsStrictFPEnabled = true;
+
   // Function alignments.
   const Align FunctionAlignment(Subtarget.hasStdExtC() ? 2 : 4);
   setMinFunctionAlignment(FunctionAlignment);
   setPrefFunctionAlignment(FunctionAlignment);
 
   setMinimumJumpTableEntries(5);
 
   // Jumps are expensive, compared to logic