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

[asb]

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.

[eopXD]

We probably should do the same to HasRISCVVTypes and HasFloat16.

[kito-cheng]

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]

clang/lib/Basic/Targets/RISCV.cpp

323

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

[asb]

In D145071#4161744, @eopXD wrote:

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

[zahiraam]

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

324

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]

clang/lib/Basic/Targets/RISCV.cpp

323

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

[craig.topper]

LGTM

[Yunzezhu]

Could you please upstream this revision? Thanks. @asb

clang/lib/Basic/Targets/RISCV.cpp

if (!ParseResult) {
     return false;
   } else {
     ISAInfo = std::move(*ParseResult);
   }
 
   if (ABI.empty())
     ABI = ISAInfo->computeDefaultABI().str();
 
+  if (ISAInfo->hasExtension("zfh"))
+    HasLegalHalfType = true;
+
   return true;
 }
 
 bool RISCVTargetInfo::isValidCPUName(StringRef Name) const {
   bool Is64Bit = getTriple().isArch64Bit();
   return llvm::RISCV::parseCPU(Name, Is64Bit);
 }
 

clang/test/CodeGen/RISCV/Float16-arith.c

 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 2
 // RUN: %clang_cc1 -triple riscv32 -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefix=NOZFH %s
 // RUN: %clang_cc1 -triple riscv64 -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefix=NOZFH %s
 // RUN: %clang_cc1 -triple riscv32 -target-feature +zfh -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefix=ZFH %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +zfh -emit-llvm %s -o - \
 // RUN:   | FileCheck -check-prefix=ZFH %s
 
 _Float16 x, y, z;
 
 // With no native half type support (no zfh), f16 will be promoted to f32.
-// With zfh, it shouldn't be (FIXME: set HasLegalHalfType = true in order to
-// get this behaviour for zfh).
+// With zfh, it shouldn't be.
 
 // NOZFH-LABEL: define dso_local void @f16_add
 // NOZFH-SAME: () #[[ATTR0:[0-9]+]] {
 // NOZFH-NEXT:  entry:
 // NOZFH-NEXT:    [[TMP0:%.*]] = load half, ptr @y, align 2
 // NOZFH-NEXT:    [[EXT:%.*]] = fpext half [[TMP0]] to float
 // NOZFH-NEXT:    [[TMP1:%.*]] = load half, ptr @z, align 2
 // NOZFH-NEXT:    [[EXT1:%.*]] = fpext half [[TMP1]] to float
 // NOZFH-NEXT:    [[ADD:%.*]] = fadd float [[EXT]], [[EXT1]]
 // NOZFH-NEXT:    [[UNPROMOTION:%.*]] = fptrunc float [[ADD]] to half
 // NOZFH-NEXT:    store half [[UNPROMOTION]], ptr @x, align 2
 // NOZFH-NEXT:    ret void
 //
 // ZFH-LABEL: define dso_local void @f16_add
 // ZFH-SAME: () #[[ATTR0:[0-9]+]] {
 // ZFH-NEXT:  entry:
 // ZFH-NEXT:    [[TMP0:%.*]] = load half, ptr @y, align 2
-// ZFH-NEXT:    [[EXT:%.*]] = fpext half [[TMP0]] to float
 // ZFH-NEXT:    [[TMP1:%.*]] = load half, ptr @z, align 2
-// ZFH-NEXT:    [[EXT1:%.*]] = fpext half [[TMP1]] to float
-// ZFH-NEXT:    [[ADD:%.*]] = fadd float [[EXT]], [[EXT1]]
-// ZFH-NEXT:    [[UNPROMOTION:%.*]] = fptrunc float [[ADD]] to half
-// ZFH-NEXT:    store half [[UNPROMOTION]], ptr @x, align 2
+// ZFH-NEXT:    [[ADD:%.*]] = fadd half [[TMP0]], [[TMP1]]
+// ZFH-NEXT:    store half [[ADD]], ptr @x, align 2
 // ZFH-NEXT:    ret void
 //
 void f16_add() {
   x = y + z;
 }