kito-cheng added inline comments.
================
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:
> > 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.
I think check `zve32f` would be better? `zve32x` means we have integer vector
instruction but might not have any floating point vector instructions.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130311/new/
https://reviews.llvm.org/D130311
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
