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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:286
+  // StrictFP support for vectors is incomplete.
+  if (ISAInfo->hasExtension("zve32x"))
+    HasStrictFP = false;
----------------
reames wrote:
> craig.topper wrote:
> > reames wrote:
> > > 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.)
> > 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."
> 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.  :)
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130311