Page MenuHomePhabricator

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

Authored by craig.topper on Jul 21 2022, 3:01 PM.

Details

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.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 21 2022, 3:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 3:01 PM
craig.topper requested review of this revision.Jul 21 2022, 3:01 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 21 2022, 3:01 PM
craig.topper added inline comments.Jul 21 2022, 3:04 PM
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 added inline comments.Jul 21 2022, 3:06 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
933

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 added a comment.Aug 1 2022, 3:23 AM

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 added a comment.Aug 1 2022, 8:12 AM

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.

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.

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 added inline comments.Aug 1 2022, 11:31 AM
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 added inline comments.Aug 1 2022, 11:35 AM
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 added inline comments.Aug 1 2022, 3:36 PM
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.

Add hasVInstructions to RISCVISAInfo

kito-cheng added inline comments.Aug 1 2022, 11:41 PM
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 added inline comments.Aug 2 2022, 1:40 AM
llvm/lib/Support/RISCVISAInfo.cpp
232 ↗(On Diff #449150)

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