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

[Philip Reames via Phabricator via cfe-commits]

reames added a comment.

In D130311#3691146 <https://reviews.llvm.org/D130311#3691146>, @craig.topper wrote:

> In D130311#3691029 <https://reviews.llvm.org/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.)



================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:286
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasExtension("zve32x"))
+    HasStrictFP = false;
----------------
craig.topper wrote:
> reames wrote:
> > asb wrote:
> > > 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.
> > 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.  
> V implies Zve64d implies Zve64f implies Zve32f and Zve64x. Zve32f implies Zve32x. Zve32x is the root of the vector inheritance tree.
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130311/new/

https://reviews.llvm.org/D130311