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:
> > > 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."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130311

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to