This is an archive of the discontinued LLVM Phabricator instance.

[clang][RISCV] Set HasLegalHalfType to true if zfh is enabled
ClosedPublic

Authored by asb on Mar 1 2023, 7:32 AM.

Details

Summary

The desired semantics for HasLegalHalfType are slightly unclear in that the comment for HasLegalHalfType says "True if the backend supports operations on the half LLVM IR type." Which operations? We get very limited scalar operations with zfhmin, more with zfh, and vector support with zvfh. While the comment for hasLegalHalfType() says "Determine whether _Float16 is supported on this target."

The only target-independent use of hasLegalHalfType appears to be in the logic for UseExcessPrecision added in D136176.

Sharing this trivial path for review for a second opinion on what the "correct" answer is for RISC-V. If we agree this is correct, this patch should likely be updated with a test that tries to demonstrate a difference in -fexcess-precision logic.

Diff Detail

Event Timeline

asb created this revision.Mar 1 2023, 7:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2023, 7:32 AM
asb requested review of this revision.Mar 1 2023, 7:32 AM
eopXD added a comment.Mar 1 2023, 8:19 AM

We probably should do the same to HasRISCVVTypes and HasFloat16.

I would say it should always set to true if I just read first paragraph, since we have softfloat for fp16.

But I think it should be what you proposed according the UseExcessPrecision implementation.

And maybe we need a clang test for that like clang/test/CodeGen/X86/fexcess-precision.c?


BTW, I've created a bug for AArch64 long times ago when I implement and testing zfh implementation on GCC, https://github.com/llvm/llvm-project/issues/42820 @SjoerdMeijer ARM guys might interested on that.

kito-cheng added inline comments.Mar 1 2023, 9:12 AM
clang/lib/Basic/Targets/RISCV.cpp
318

Oh, don't forgot zhinx, I almost forgot that too.

asb added a comment.Mar 1 2023, 9:28 AM

We probably should do the same to HasRISCVVTypes and HasFloat16.

I haven't looked at HasRISCVVTypes, but I did work through all the f16 related hooks I could find, and I think we're doing the right thing for HasFloat16. Setting it to false will reject _Float16 with a Sema diagnostic, which I don't think is what we want and is also not consistent with other platforms that support f16 (we do lower to the appropriate libcalls on the backend, so can handle it just fine). HasFloat16 is unconditionally true for Arm and only gated on SSE2 support for X86

This seems to be in contrast to HasLegalHalfType, which appears to be about whether native instructions to handle it are present (such as avx512fp16 on X86).

HasFloat16 is unconditionally true for Arm and only gated on SSE2 support for X86

My understanding of that is because all aarch64 implementations support ARMv8 FP which makes half type for this target legal. Setting both HasLegalHalfType and HasFloat16 makes sense.
That's not the case for x86. HasLegalHalfType needs the avx512fp16 feature to be set.

clang/lib/Basic/Targets/RISCV.cpp
319

If your goal is to avoid excess precision with the target having this feature, then this is the right thing to do.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/RISCV.h#L43 will set HasFloat16=true.

craig.topper added inline comments.Mar 8 2023, 9:28 AM
clang/lib/Basic/Targets/RISCV.cpp
318

I don't think CodeGen for zhinx is done yet.

This revision is now accepted and ready to land.Mar 8 2023, 9:28 AM
zahiraam accepted this revision.Mar 8 2023, 10:35 AM

Could you please upstream this revision? Thanks. @asb

kito-cheng accepted this revision.Apr 28 2023, 12:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 5:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript