[SystemZ] Add support for half (fp16) #109164
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-selectiondag @llvm/pr-subscribers-clang Author: Jonas Paulsson (JonPsson1) ChangesMake sure that fp16<=>float conversions are expanded to libcalls and that 16-bit fp values can be loaded and stored properly via GPRs. With this patch the Half IR Type used in operations should be handled correctly with the help of pre-existing ISD node expansions. Patch in progress... Notes: Full diff: https://github.com/llvm/llvm-project/pull/109164.diff 3 Files Affected:
diff --git a/clang/lib/Basic/Targets/SystemZ.h b/clang/lib/Basic/Targets/SystemZ.h
index f05ea473017bec..6566b63d4587ee 100644
--- a/clang/lib/Basic/Targets/SystemZ.h
+++ b/clang/lib/Basic/Targets/SystemZ.h
@@ -91,11 +91,20 @@ class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
"-v128:64-a:8:16-n32:64");
}
MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 128;
+
+ HasLegalHalfType = false; // Default=false
+ HalfArgsAndReturns = false; // Default=false
+ HasFloat16 = true; // Default=false
+
HasStrictFP = true;
}
unsigned getMinGlobalAlign(uint64_t Size, bool HasNonWeakDef) const override;
+ bool useFP16ConversionIntrinsics() const override {
+ return false;
+ }
+
void getTargetDefines(const LangOptions &Opts,
MacroBuilder &Builder) const override;
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 582a8c139b2937..fd3dcebba1eca7 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -704,6 +704,13 @@ SystemZTargetLowering::SystemZTargetLowering(const TargetMachine &TM,
setOperationAction(ISD::BITCAST, MVT::f32, Custom);
}
+ // Expand FP16 <=> FP32 conversions to libcalls and handle FP16 loads and
+ // stores in GPRs.
+ setOperationAction(ISD::FP16_TO_FP, MVT::f32, Expand);
+ setOperationAction(ISD::FP_TO_FP16, MVT::f32, Expand);
+ setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f16, Expand);
+ setTruncStoreAction(MVT::f32, MVT::f16, Expand);
+
// VASTART and VACOPY need to deal with the SystemZ-specific varargs
// structure, but VAEND is a no-op.
setOperationAction(ISD::VASTART, MVT::Other, Custom);
diff --git a/llvm/test/CodeGen/SystemZ/fp-half.ll b/llvm/test/CodeGen/SystemZ/fp-half.ll
new file mode 100644
index 00000000000000..393ba2f620ff6e
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/fp-half.ll
@@ -0,0 +1,100 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z10 | FileCheck %s
+;
+; Tests for FP16 (Half).
+
+; A function where everything is done in Half.
+define void @fun0(ptr %Op0, ptr %Op1, ptr %Dst) {
+; CHECK-LABEL: fun0:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: stmg %r12, %r15, 96(%r15)
+; CHECK-NEXT: .cfi_offset %r12, -64
+; CHECK-NEXT: .cfi_offset %r13, -56
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -168
+; CHECK-NEXT: .cfi_def_cfa_offset 328
+; CHECK-NEXT: std %f8, 160(%r15) # 8-byte Folded Spill
+; CHECK-NEXT: .cfi_offset %f8, -168
+; CHECK-NEXT: llgh %r2, 0(%r2)
+; CHECK-NEXT: lgr %r13, %r4
+; CHECK-NEXT: lgr %r12, %r3
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: llgh %r2, 0(%r12)
+; CHECK-NEXT: ler %f8, %f0
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: aebr %f0, %f8
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: sth %r2, 0(%r13)
+; CHECK-NEXT: ld %f8, 160(%r15) # 8-byte Folded Reload
+; CHECK-NEXT: lmg %r12, %r15, 264(%r15)
+; CHECK-NEXT: br %r14
+entry:
+ %0 = load half, ptr %Op0, align 2
+ %1 = load half, ptr %Op1, align 2
+ %add = fadd half %0, %1
+ store half %add, ptr %Dst, align 2
+ ret void
+}
+
+; A function where Half values are loaded and extended to float and then
+; operated on.
+define void @fun1(ptr %Op0, ptr %Op1, ptr %Dst) {
+; CHECK-LABEL: fun1:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: stmg %r12, %r15, 96(%r15)
+; CHECK-NEXT: .cfi_offset %r12, -64
+; CHECK-NEXT: .cfi_offset %r13, -56
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -168
+; CHECK-NEXT: .cfi_def_cfa_offset 328
+; CHECK-NEXT: std %f8, 160(%r15) # 8-byte Folded Spill
+; CHECK-NEXT: .cfi_offset %f8, -168
+; CHECK-NEXT: llgh %r2, 0(%r2)
+; CHECK-NEXT: lgr %r13, %r4
+; CHECK-NEXT: lgr %r12, %r3
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: llgh %r2, 0(%r12)
+; CHECK-NEXT: ler %f8, %f0
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: aebr %f0, %f8
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: sth %r2, 0(%r13)
+; CHECK-NEXT: ld %f8, 160(%r15) # 8-byte Folded Reload
+; CHECK-NEXT: lmg %r12, %r15, 264(%r15)
+; CHECK-NEXT: br %r14
+entry:
+ %0 = load half, ptr %Op0, align 2
+ %ext = fpext half %0 to float
+ %1 = load half, ptr %Op1, align 2
+ %ext1 = fpext half %1 to float
+ %add = fadd float %ext, %ext1
+ %res = fptrunc float %add to half
+ store half %res, ptr %Dst, align 2
+ ret void
+}
+
+; Test case with a Half incoming argument.
+define zeroext i1 @fun2(half noundef %f) {
+; CHECK-LABEL: fun2:
+; CHECK: # %bb.0: # %start
+; CHECK-NEXT: stmg %r14, %r15, 112(%r15)
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -160
+; CHECK-NEXT: .cfi_def_cfa_offset 320
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: larl %r1, .LCPI2_0
+; CHECK-NEXT: deb %f0, 0(%r1)
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: risbg %r2, %r2, 63, 191, 49
+; CHECK-NEXT: lmg %r14, %r15, 272(%r15)
+; CHECK-NEXT: br %r14
+start:
+ %self = fdiv half %f, 0xHC700
+ %_4 = bitcast half %self to i16
+ %_0 = icmp slt i16 %_4, 0
+ ret i1 %_0
+}
|
|
@llvm/pr-subscribers-backend-systemz Author: Jonas Paulsson (JonPsson1) ChangesMake sure that fp16<=>float conversions are expanded to libcalls and that 16-bit fp values can be loaded and stored properly via GPRs. With this patch the Half IR Type used in operations should be handled correctly with the help of pre-existing ISD node expansions. Patch in progress... Notes: Full diff: https://github.com/llvm/llvm-project/pull/109164.diff 3 Files Affected:
diff --git a/clang/lib/Basic/Targets/SystemZ.h b/clang/lib/Basic/Targets/SystemZ.h
index f05ea473017bec..6566b63d4587ee 100644
--- a/clang/lib/Basic/Targets/SystemZ.h
+++ b/clang/lib/Basic/Targets/SystemZ.h
@@ -91,11 +91,20 @@ class LLVM_LIBRARY_VISIBILITY SystemZTargetInfo : public TargetInfo {
"-v128:64-a:8:16-n32:64");
}
MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 128;
+
+ HasLegalHalfType = false; // Default=false
+ HalfArgsAndReturns = false; // Default=false
+ HasFloat16 = true; // Default=false
+
HasStrictFP = true;
}
unsigned getMinGlobalAlign(uint64_t Size, bool HasNonWeakDef) const override;
+ bool useFP16ConversionIntrinsics() const override {
+ return false;
+ }
+
void getTargetDefines(const LangOptions &Opts,
MacroBuilder &Builder) const override;
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 582a8c139b2937..fd3dcebba1eca7 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -704,6 +704,13 @@ SystemZTargetLowering::SystemZTargetLowering(const TargetMachine &TM,
setOperationAction(ISD::BITCAST, MVT::f32, Custom);
}
+ // Expand FP16 <=> FP32 conversions to libcalls and handle FP16 loads and
+ // stores in GPRs.
+ setOperationAction(ISD::FP16_TO_FP, MVT::f32, Expand);
+ setOperationAction(ISD::FP_TO_FP16, MVT::f32, Expand);
+ setLoadExtAction(ISD::EXTLOAD, MVT::f32, MVT::f16, Expand);
+ setTruncStoreAction(MVT::f32, MVT::f16, Expand);
+
// VASTART and VACOPY need to deal with the SystemZ-specific varargs
// structure, but VAEND is a no-op.
setOperationAction(ISD::VASTART, MVT::Other, Custom);
diff --git a/llvm/test/CodeGen/SystemZ/fp-half.ll b/llvm/test/CodeGen/SystemZ/fp-half.ll
new file mode 100644
index 00000000000000..393ba2f620ff6e
--- /dev/null
+++ b/llvm/test/CodeGen/SystemZ/fp-half.ll
@@ -0,0 +1,100 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=s390x-linux-gnu -mcpu=z10 | FileCheck %s
+;
+; Tests for FP16 (Half).
+
+; A function where everything is done in Half.
+define void @fun0(ptr %Op0, ptr %Op1, ptr %Dst) {
+; CHECK-LABEL: fun0:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: stmg %r12, %r15, 96(%r15)
+; CHECK-NEXT: .cfi_offset %r12, -64
+; CHECK-NEXT: .cfi_offset %r13, -56
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -168
+; CHECK-NEXT: .cfi_def_cfa_offset 328
+; CHECK-NEXT: std %f8, 160(%r15) # 8-byte Folded Spill
+; CHECK-NEXT: .cfi_offset %f8, -168
+; CHECK-NEXT: llgh %r2, 0(%r2)
+; CHECK-NEXT: lgr %r13, %r4
+; CHECK-NEXT: lgr %r12, %r3
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: llgh %r2, 0(%r12)
+; CHECK-NEXT: ler %f8, %f0
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: aebr %f0, %f8
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: sth %r2, 0(%r13)
+; CHECK-NEXT: ld %f8, 160(%r15) # 8-byte Folded Reload
+; CHECK-NEXT: lmg %r12, %r15, 264(%r15)
+; CHECK-NEXT: br %r14
+entry:
+ %0 = load half, ptr %Op0, align 2
+ %1 = load half, ptr %Op1, align 2
+ %add = fadd half %0, %1
+ store half %add, ptr %Dst, align 2
+ ret void
+}
+
+; A function where Half values are loaded and extended to float and then
+; operated on.
+define void @fun1(ptr %Op0, ptr %Op1, ptr %Dst) {
+; CHECK-LABEL: fun1:
+; CHECK: # %bb.0: # %entry
+; CHECK-NEXT: stmg %r12, %r15, 96(%r15)
+; CHECK-NEXT: .cfi_offset %r12, -64
+; CHECK-NEXT: .cfi_offset %r13, -56
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -168
+; CHECK-NEXT: .cfi_def_cfa_offset 328
+; CHECK-NEXT: std %f8, 160(%r15) # 8-byte Folded Spill
+; CHECK-NEXT: .cfi_offset %f8, -168
+; CHECK-NEXT: llgh %r2, 0(%r2)
+; CHECK-NEXT: lgr %r13, %r4
+; CHECK-NEXT: lgr %r12, %r3
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: llgh %r2, 0(%r12)
+; CHECK-NEXT: ler %f8, %f0
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: aebr %f0, %f8
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: sth %r2, 0(%r13)
+; CHECK-NEXT: ld %f8, 160(%r15) # 8-byte Folded Reload
+; CHECK-NEXT: lmg %r12, %r15, 264(%r15)
+; CHECK-NEXT: br %r14
+entry:
+ %0 = load half, ptr %Op0, align 2
+ %ext = fpext half %0 to float
+ %1 = load half, ptr %Op1, align 2
+ %ext1 = fpext half %1 to float
+ %add = fadd float %ext, %ext1
+ %res = fptrunc float %add to half
+ store half %res, ptr %Dst, align 2
+ ret void
+}
+
+; Test case with a Half incoming argument.
+define zeroext i1 @fun2(half noundef %f) {
+; CHECK-LABEL: fun2:
+; CHECK: # %bb.0: # %start
+; CHECK-NEXT: stmg %r14, %r15, 112(%r15)
+; CHECK-NEXT: .cfi_offset %r14, -48
+; CHECK-NEXT: .cfi_offset %r15, -40
+; CHECK-NEXT: aghi %r15, -160
+; CHECK-NEXT: .cfi_def_cfa_offset 320
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: brasl %r14, __gnu_h2f_ieee@PLT
+; CHECK-NEXT: larl %r1, .LCPI2_0
+; CHECK-NEXT: deb %f0, 0(%r1)
+; CHECK-NEXT: brasl %r14, __gnu_f2h_ieee@PLT
+; CHECK-NEXT: risbg %r2, %r2, 63, 191, 49
+; CHECK-NEXT: lmg %r14, %r15, 272(%r15)
+; CHECK-NEXT: br %r14
+start:
+ %self = fdiv half %f, 0xHC700
+ %_4 = bitcast half %self to i16
+ %_0 = icmp slt i16 %_4, 0
+ ret i1 %_0
+}
|
You can test this locally with the following command:git-clang-format --diff eef5ea0c42fc07ef2c948be59b57d0df8ec801ca 159f70d573dd1ffce155633e3c99ec8b563d4604 --extensions h,c,cpp -- clang/test/CodeGen/SystemZ/Float16.c clang/test/CodeGen/SystemZ/fp16.c compiler-rt/lib/builtins/extendhfdf2.c compiler-rt/test/builtins/Unit/extendhfdf2_test.c clang/include/clang/Basic/TargetInfo.h clang/lib/Basic/Targets/SystemZ.h clang/lib/CodeGen/Targets/SystemZ.cpp clang/test/CodeGen/SystemZ/strictfp_builtins.c clang/test/CodeGen/SystemZ/systemz-abi.c clang/test/CodeGen/SystemZ/systemz-inline-asm.c compiler-rt/lib/builtins/clear_cache.c llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp llvm/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.cpp llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCTargetDesc.h llvm/lib/Target/SystemZ/SystemZAsmPrinter.cpp llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp llvm/lib/Target/SystemZ/SystemZISelLowering.cpp llvm/lib/Target/SystemZ/SystemZISelLowering.h llvm/lib/Target/SystemZ/SystemZInstrInfo.cpp llvm/lib/Target/SystemZ/SystemZRegisterInfo.cpp llvm/lib/Target/SystemZ/SystemZRegisterInfo.hView the diff from clang-format here.diff --git a/compiler-rt/test/builtins/Unit/extendhfdf2_test.c b/compiler-rt/test/builtins/Unit/extendhfdf2_test.c
index 422e272c11..bf33291d8d 100644
--- a/compiler-rt/test/builtins/Unit/extendhfdf2_test.c
+++ b/compiler-rt/test/builtins/Unit/extendhfdf2_test.c
@@ -7,81 +7,63 @@
double __extendhfdf2(TYPE_FP16 a);
-int test__extendhfdf2(TYPE_FP16 a, uint64_t expected)
-{
- double x = __extendhfdf2(a);
- int ret = compareResultD(x, expected);
+int test__extendhfdf2(TYPE_FP16 a, uint64_t expected) {
+ double x = __extendhfdf2(a);
+ int ret = compareResultD(x, expected);
- if (ret){
- printf("error in test__extendhfdf2(%#.4x) = %f, "
- "expected %f\n", toRep16(a), x, fromRep64(expected));
- }
- return ret;
+ if (ret) {
+ printf("error in test__extendhfdf2(%#.4x) = %f, "
+ "expected %f\n",
+ toRep16(a), x, fromRep64(expected));
+ }
+ return ret;
}
char assumption_1[sizeof(TYPE_FP16) * CHAR_BIT == 16] = {0};
-int main()
-{
- // qNaN
- if (test__extendhfdf2(makeQNaN16(),
- UINT64_C(0x7ff8000000000000)))
- return 1;
- // NaN
- if (test__extendhfdf2(fromRep16(0x7f80),
- UINT64_C(0x7ffe000000000000)))
- return 1;
- // inf
- if (test__extendhfdf2(makeInf16(),
- UINT64_C(0x7ff0000000000000)))
- return 1;
- // -inf
- if (test__extendhfdf2(makeNegativeInf16(),
- UINT64_C(0xfff0000000000000)))
- return 1;
- // zero
- if (test__extendhfdf2(fromRep16(0x0),
- UINT64_C(0x0)))
- return 1;
- // -zero
- if (test__extendhfdf2(fromRep16(0x8000),
- UINT64_C(0x8000000000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x4248),
- UINT64_C(0x4009200000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0xc248),
- UINT64_C(0xc009200000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x6e62),
- UINT64_C(0x40b9880000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x3c00),
- UINT64_C(0x3ff0000000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x0400),
- UINT64_C(0x3f10000000000000)))
- return 1;
- // denormal
- if (test__extendhfdf2(fromRep16(0x0010),
- UINT64_C(0x3eb0000000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x0001),
- UINT64_C(0x3e70000000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x8001),
- UINT64_C(0xbe70000000000000)))
- return 1;
- if (test__extendhfdf2(fromRep16(0x0001),
- UINT64_C(0x3e70000000000000)))
- return 1;
- // max (precise)
- if (test__extendhfdf2(fromRep16(0x7bff),
- UINT64_C(0x40effc0000000000)))
- return 1;
- // max (rounded)
- if (test__extendhfdf2(fromRep16(0x7bff),
- UINT64_C(0x40effc0000000000)))
- return 1;
- return 0;
+int main() {
+ // qNaN
+ if (test__extendhfdf2(makeQNaN16(), UINT64_C(0x7ff8000000000000)))
+ return 1;
+ // NaN
+ if (test__extendhfdf2(fromRep16(0x7f80), UINT64_C(0x7ffe000000000000)))
+ return 1;
+ // inf
+ if (test__extendhfdf2(makeInf16(), UINT64_C(0x7ff0000000000000)))
+ return 1;
+ // -inf
+ if (test__extendhfdf2(makeNegativeInf16(), UINT64_C(0xfff0000000000000)))
+ return 1;
+ // zero
+ if (test__extendhfdf2(fromRep16(0x0), UINT64_C(0x0)))
+ return 1;
+ // -zero
+ if (test__extendhfdf2(fromRep16(0x8000), UINT64_C(0x8000000000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x4248), UINT64_C(0x4009200000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0xc248), UINT64_C(0xc009200000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x6e62), UINT64_C(0x40b9880000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x3c00), UINT64_C(0x3ff0000000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x0400), UINT64_C(0x3f10000000000000)))
+ return 1;
+ // denormal
+ if (test__extendhfdf2(fromRep16(0x0010), UINT64_C(0x3eb0000000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x0001), UINT64_C(0x3e70000000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x8001), UINT64_C(0xbe70000000000000)))
+ return 1;
+ if (test__extendhfdf2(fromRep16(0x0001), UINT64_C(0x3e70000000000000)))
+ return 1;
+ // max (precise)
+ if (test__extendhfdf2(fromRep16(0x7bff), UINT64_C(0x40effc0000000000)))
+ return 1;
+ // max (rounded)
+ if (test__extendhfdf2(fromRep16(0x7bff), UINT64_C(0x40effc0000000000)))
+ return 1;
+ return 0;
}
|
|
Note that you need to also have softPromoteHalfType return true to get correct legalization for half operations. |
Thanks for pointing that out - patch updated. |
|
I think we should define and implement a proper ABI for the half type as well. |
|
Patch updated after some progress... With this version, the fp16 values are passed to conversion functions as integer, which seems to be the default. It is however a bit tricky to do this and at the same time pass half values in FP registers. At this point I wonder for one thing if it would be better to pass FP16 values to the conversion functions as _Float16 instead? It seems this may be possible to change in the configurations by looking at COMPILER_RT_HAS_FLOAT16 / compiler-rt/lib/builtins/extendhfsf2.c / fp_extend.h... Not really sure if those conversion functions are supposed to be built and only used for soft-promotion of fp16, or if there are any external implications, for instance gcc compatability. Any other comments also welcome... |
My understanding is that in GCC's From your first two sentences it sounds like @uweigand mentioned figuring out an ABI for A quick check seems to show that GCC 13 does not support Note that there are some common issues with these conversions, would probably be good to test against them if possible #97981 #97975. |
From what I can see in the libgcc sources, I never see
Yes, we're working on that. What we're planning to do is to have
Yes, we'd have to add those. I don't think we want
Thanks for pointing this out! |
I think this is accurate, libgcc just appears to (reasonably) not provide any f16-related symbols on platforms where GCC doesn't support For that reason we just always provide the symbols in rust's compiler-builtins (though we let LLVM figure out that
That is great news, especially considering how problematic the target-feature-dependent ABI on x86-32 has been. |
|
Patch reworked:
(twoaddr-kill.mir test updated as the hard-coded register class enum value for GRH32BitRegClass has changed.) Still some more points to go over, but it seems to be working fairly well at this point.
|
|
Patch improved further:
|
| setOperationAction(ISD::FCOS, VT, Expand); | ||
| setOperationAction(ISD::FSINCOS, VT, Expand); | ||
| setOperationAction(ISD::FREM, VT, Expand); | ||
| setOperationAction(ISD::FPOW, VT, Expand); |
There was a problem hiding this comment.
Shouldn't these be Promote just like all the other f16 operations? Expand triggers a libcall, which doesn't match the excess-precision setting - also, we actually don't have f16 libcalls in libm ...
There was a problem hiding this comment.
ok, if there are no f16 libcalls it works to have them be promoted.
There was a problem hiding this comment.
Just crosslinking that there is an effort to add f16 libcalls #95250 but I have no clue what the plan is as far as lowering to them.
|
I think that compiler-rt should provide all six conversion routines (f16<->f32, f16<->f64, f16<->f128), simply because libgcc has them and GCC-generated code will use them, and compiler-rt is supposed to be usable as drop-in replacement. Also, I think LLVM should ideally generate calls to all those six routines, like GCC does. Not only is this probably the most efficient (the f16<->f32 and f16<->f64 routines should be pretty much equal in performance), but more importantly, in the truncate case, using only a single truncation is more exact due to avoiding double-rounding effects. |
Yes, we support soft-float, and as I wrote earlier I am worried what happens with soft-float and conversion functions. I saw that with soft-float, the same conversion functions are called, but with args in integer registers, while with hard-float they go in fp-registers. For that to work, there would have to be another library with the same functions but with the soft-float abi. I am not sure if compiler-rt could be built for soft-float, but perhaps the gcc lib has them, in which case it seem reasonable to allow that ABI change in the tests. The user just has to provide a soft-float library for the conversion functions as well. I guess we should provide all six conversion functions (for hard float) then, per your comments. |
|
To clarify about soft-float - we do support There is no user-space code that uses To bring this back to the original topic of floating-point support in the compiler runtime libraries: there are a small number of such routines today, but those are not intended for use with soft-float (they do expect the normal hard-float ABI, i.e. passing floating-point values in floating-point registers), but rather implement a few corner-case operations (like int128 <-> float conversion) where we actually do not have hardware instructions. Any new f16-related compiler runtime library routine would fall into this category, not the soft-float category. |
|
If there isn't any reason to be consistent with the other LLVM targets then agreed, using the direct libcalls seems better. The new library support could likely land separately, right? As long as the lowering is correct, considering this PR is already pretty expansive.
From https://llvm.godbolt.org/z/sr6aW5e1e it looks like
For reference, this is what we do for Rust. Some more details, as an aside: We have started splitting There isn't actually a softfloat 390x target currently but this will need to be added at some point for RfL support (it's trivial). |
No, XF is a different mode from DF. There should be a file This seems to be missing currently.
This should be defined by the existing logic in and both conditions should be true on s390x. On the one hand, we have and we should be defining On the other hand, we have and we should match the So this should already work. If it doesn't, you'll have to debug the build process here (e.g. look at preprocessed output to see what's actually going on here).
I'm pretty sure that would be the same
Sure, that probably should land separately. I'd still like to at least have an implementation ahead of time just so we can properly test the LLVM back-end support ...
Thanks for pointing this out. I agree, we need that target for s390x then as well. (And probably soon - the kernel folks are already trying to enable Rust for Linux on s390x.) |
Ah, sorry, now I see what's going on. In this line you added to the cmake logic: you need to add |
|
Thanks for the help.
Still problem with wrong-code: All conversions gave wrong-code when building compiler-rt with gcc, but when bootstrapping it (by using -DLLVM_ENABLE_RUNTIMES=compiler-rt), now only the double case fails. The clang assembly files are identical, but the libfunctions are different when I compared with libgcc (correct output) and compiler-rt. I stepped through my small program in gdb and found that as expected when returning from libgcc after the first extension, the register holds 1.5. When doing the same from compiler-rt it doesn't have the correct value. I checked that the ABI is correct at least - first instruction is moving f0 to r0, which seems right. At this point I wouldn't know what to look for to debug it further. My test program: |
|
To clarify, the code is calling Not sure if you are testing only on s390x but there should probably be a unit test at https://github.com/llvm/llvm-project/tree/d90423e310482bdbc731242fa25dcb3dd44e69de/compiler-rt/test/builtins/Unit to see if things work on other platforms. |
Yes, my test program is calling either libgcc or compiler-rt implementaions of the same function. Yes, on this branch.
I am testing on s390x, and I guess it's unfortunate that there is no such test for hfdf - this is the file that was missing and that I added. I guess it should be added once it is confirmed to work properly (or if someone would provide the right values). The tests that are already there have been passing always while working on this, even when building with gcc. |
|
Nothing about the implementation stands out to me as wrong. It looks like the exponent isn't correct, I guess you could step through this portion llvm-project/compiler-rt/lib/builtins/fp_extend_impl.inc Lines 65 to 66 in ae42f07 By the way, these sites are pretty helpful for double checking float reprs https://float.exposed https://weitz.de/ieee/. |
This is set if the host compiler used to build compiler-rt supports the If this is not set, you need to figure out why it is not using the correct host compiler. I think it should be using the clang built from the same sources, which should now support |
| return __extendXfYf2__(a); | ||
| } | ||
|
|
||
| COMPILER_RT_ABI float __gnu_h2d_ieee(src_t a) { return __extendhfdf2(a); } |
There was a problem hiding this comment.
I don't think you should define the __gnu_h2d_ieee or the __aeabi_h2d names here - those are not defined by the ARM standard or by other compilers today. This function should only be present under the __extendhfdf2 name.
It seems that with a completely new build with the new cmake config the test actually passes: I previously got confused here after rerunning cmake in the same folder as I thought it was enough to see the newly built clang being used (ninja -v) with compiler-rt. It seems maybe cmake itself got confused and used system gcc for its checks while building with clang? Anyway, it works now, sorry. I also found the problem with the miscompile (after some debugging, sigh), that the file I copied (extendhfsf.c) has float hard coded as return type, even though it defines DST_SINGLE. extendhftf2.c uses dst_t as return type, but I see other files using the type directly, so I will leave those files and extendhftf2.c as they were. I guess I have a motivating case to use dst_t and src_t everywhere, but that's another issue. So the miscompile was due to a final conversion from double to float before returning. My bad. While adding the corresponding (to extendsfdf2_test.c) test I tried inserting an obvious error, but this did not cause it to fail, not even with ninja check-all, which I thought covered all tests (ninja check-compiler-rt did not report it either). How can I run these tests? I tried building (same way as before) and running the existing extendhfsf2_test.c but got:
This was unexpected, and I get the same error with libgcc.
I tried float.exposed but I couldn't really convert an f16 hex to a double hex. Is it supposed to be able to do this? |
It should, on the |
|
-- A few more tests added covering Would a test for { half, half, half, half } in Swift-return.ll make sense? I think testing coverage now is fairly ok - can't think of any more instructions to handle although not all various combinations and uses of them have been duplicated from float (Test for the new extendhfdf2.cpp function is still todo). -- Manged to build and run lbm with _Float16 by simply changing the element type o LBM_Grid from double to _Float16. It ran without crashing, although 4x quicker which I guess/hope is due to some value check that stops execution somewhere. I confirmed that the executable indeed contains a lot of conversion calls and vlreph and more. -- Another try to activate the tests for the builtins, but so far no luck. In compiler-rt/test/builtins/CMakeLists.txt, the librt_has_XXX list is built, but I tried but no failure... |
Experiment with soft-promotion in FP regs (not working). Try to make f16 legal instead Atomic loads/stores, spill/reload, tests for __fp16 and half vectors. strict f16 with tests. Review Make use of vector facility if present.
Use 4-byte spill size in case of no vector support. Build the generic compiler-rt sources for s390x. Don't set libcall names. @llvm.s390.tdc, fcopysign, strict_fminimum/fmaximum. More tests for f16, but not complete. libfuncs built also to double and long double.
|
@Meinersbur @petrhosek @smeenai @danliew-apple Reaching out here to some people who seem to have been working with this CMake file: Would you have any idea how I could activate and run the compiler-rt tests for s390x (see above for my unsuccessful experiments)? Thanks. |
|
Thanks to @boomanaiden154 for pointing out that I need to pass -DCOMPILER_RT_BUILD_BUILTINS=ON to cmake - the tests now builds and runs. The test for half->double conversions added, with the 64-bit hex values taken from the compiler-rt conversion function results (should be correct, right :). Wanted to use the makeXXX16() functions as much as possible, but not sure what to pass to makeQNaN16(uint16_t rand), so left that as it was, which also should be fine, I suppose. |
|
Have nothing more to do for this now, so waiting for review. |
| if (!useSoftFloat()) { | ||
| // Promote all f16 operations to float, with some exceptions below. | ||
| for (unsigned Opc = 0; Opc < ISD::BUILTIN_OP_END; ++Opc) | ||
| setOperationAction(Opc, MVT::f16, Promote); |
There was a problem hiding this comment.
I thought Promote for half types is broken and you need to use SoftPromoteHalf for correct rounding?
There was a problem hiding this comment.
What happens is that calls to compiler-rt are emitted for conversions and as we keep all f16 in fp-registers always, this seems to work fine.
I thought SoftPromoteHalf is for targets that use i16 for f16?
There was a problem hiding this comment.
SoftPromoteHalf is a type legalizer action not an OperationAction. Promote as an OperationAction should work for f16.
There was a problem hiding this comment.
-
LLVM miscompiles consecutive
halfoperations by using too much precision on several backends #97975 "...a conversion back to f16 must occur after each operation, otherwise the excess precision of float will affect the result...": There are already tests for this, for example @Fun6 in fp-half.ll. -
LLVM miscompiles passing/returning
halfon several backends by using lossy conversions #97981 "... The issue is that these builtins silence signalling NaNs which changes the NaN payload, meaning that the roundtrip of half -> float -> half is not lossless and causes the generated ASM to violate the semantics of LLVM IR...": Not sure what to do about this one.
There was a problem hiding this comment.
For the second one, it should be sufficient to ensure that i16<->f16 conversions are asm-only with no libcalls (it's possible this is tested somewhere and I'm just overlooking it in the large diff)
There was a problem hiding this comment.
Ah, I see. Yes, this should work then and are tested, see fp-half-move.ll.
Make sure that fp16<=>float conversions are expanded to libcalls and that 16-bit fp values can be loaded and stored properly via GPRs. With this patch the Half IR Type used in operations should be handled correctly with the help of pre-existing ISD node expansions.
Patch in progress...
Fixes #50374