Adding half float to types that can be represented by attribute((mode(xx))).
Original implementation authored by George Steed.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/test/CodeGen/aarch64-attr-mode-complex.c | ||
---|---|---|
12 | Hi Jolanta, | |
clang/test/Sema/attr-mode.c | ||
40 | shouldn't we have here and in the line below : "// expected-no-diagnostics" ? |
clang/test/Sema/attr-mode.c | ||
---|---|---|
40 | I don't think we can have both expected-error and expected-no-diagnostic in the same file. |
clang/lib/Basic/TargetInfo.cpp | ||
---|---|---|
287–288 | I *think* this is correct, but it's a bit worrying because we have multiple floating-point types with the same width. e.g., HalfTy and BFloat16Ty, so I am a bit uncomfortable with the getRealTypeByWidth() interface in general once we go down this route. getRealTypeForBitwidth() will have similar issues. | |
clang/test/CodeGen/aarch64-attr-mode-complex.c | ||
4–5 | Don't your changes also impact the behavior for mode(HF)? If so, there should be test coverage added for it. | |
clang/test/Sema/attr-mode.c | ||
40 | Nope -- that marker is only for when the entire test file expects no diagnostics. |
clang/lib/Basic/TargetInfo.cpp | ||
---|---|---|
287–288 | I think this is probably ok. If support for the other 16-bit types is needed in the future, then new mode attribute arguments will have to be specified as is done for the 128-bit types with "K", "T", and "I". |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
61 | The existing enumerators were ordered according to precision. Consider moving Half to before Float if doing so doesn't cause any problems (I would hope there is no dependence on the assigned enumerator values anywhere; if there is, then it would be helpful to add a comment about that here). |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
61 | In clang/test/CodeGenObjC/fpret.m, IR for i386-apple-darwin9 and x86_64-apple-darwin10 differ for long double after the move of Half to before Float:Half added last to the FloatModeKind enum (test PASS) generates: ; Function Attrs: noinline nounwind optnone %0 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_, align 4 %call = call float bitcast (double (i8*, i8*, ...)* @objc_msgSend_fpret to float (i8*, i8*)*)(i8* noundef null, i8* noundef %0) %1 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.2, align 4 %call1 = call double bitcast (double (i8*, i8*, ...)* @objc_msgSend_fpret to double (i8*, i8*)*)(i8* noundef null, i8* noundef %1) %2 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.4, align 4 %call2 = call x86_fp80 bitcast (double (i8*, i8*, ...)* @objc_msgSend_fpret to x86_fp80 (i8*, i8*)*)(i8* noundef null, i8* noundef %2) ret void } declare double @objc_msgSend_fpret(i8*, i8*, ...) attributes #0 = { noinline nounwind optnone "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+cx8,+x87" } While Half added before Float to the FloatModeKind enum (test FAIL) generates: ; Function Attrs: noinline nounwind optnone %0 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_, align 4 %call = call float bitcast (double (i8*, i8*, ...)* @objc_msgSend_fpret to float (i8*, i8*)*)(i8* noundef null, i8* noundef %0) %1 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.2, align 4 %call1 = call double bitcast (double (i8*, i8*, ...)* @objc_msgSend_fpret to double (i8*, i8*)*)(i8* noundef null, i8* noundef %1) %2 = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.4, align 4 %call2 = call x86_fp80 bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to x86_fp80 (i8*, i8*)*)(i8* noundef null, i8* noundef %2) ret void } declare double @objc_msgSend_fpret(i8*, i8*, ...) ; Function Attrs: nonlazybind attributes #0 = { noinline nounwind optnone "frame-pointer"="none" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+cx8,+x87" } There is a similar failure in CodeGenObjC/metadata-symbols-64.m for x86_64-apple-darwin10 where @objc_msgSend_fpret is replaced by @objc_msgSend. It looks like there are dependencies on the FloatModeKind enum values in clang/lib/Basic/Targets/X86.h and in clang/include/clang/Basic/TargetInfo.h If I shift the initial value of RealTypeUsesObjCFPRet one bit to the left the tests above will PASS, i.e this together seems to work:diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h enum class FloatModeKind { NoFloat = 255,
+ Half = 0, Double, LongDouble, Float128,
+ Ibm128 /// Fields controlling how types are laid out in memory; these may need to mutable VersionTuple PlatformMinVersion; unsigned HasAlignMac68kSupport : 1;
+ unsigned RealTypeUsesObjCFPRet : 6; unsigned ComplexLongDoubleUsesFP2Ret : 1; unsigned HasBuiltinMSVaList : 1; Is the solution, i.e. shifting the initial value of the RealTypeUsesObjCFPRet to 6, acceptable or is it too hacky? |
Addressing review comments:
- Added the test for mode(HF)
- Moved Half before Float in FloatModeKind enum class to preserve precision ordering
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222 | Good find. The implicit dependency on the FloatModeKind enumerator values makes this really fragile. Can we add some resiliency here? Perhaps:
|
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | This doesn't look right to me. The size of the bit field would be (1 << 1) | (1 << 2) which is 0b110 which is 6, but more by coincidence than by construction. I think what we want is: unsigned RealTypeUsesObjCFPRet : (int)FloatModeKind::Last + 1; |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | Sorry. I mixed things up. |
The code changes and test updates all look good to me. Please add a release note to clang/docs/ReleaseNotes.rst; I'll be happy to approve after that is done. It would be great to get an additional approval on this; I'm not an expert here.
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 |
I think this is wrong in two different ways. We need as many bits as required to store the enumerator value. The highest value is 255 (NoFloat), so we need at least 8 bits for that. But also, the last enumerator value is just that -- a value, not a width. I'd probably skip the Last entry and force the bit-field to 8 bits. | |
884 | This will assert if passed NoFloat -- is that intentional? You should also add && "some helpful message" to the assert predicate so that when the assert fails there's some more information about why the failure happened. |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | RealTypeUsesObjCFPRet is used as a bit mask indexed by the FloatModeKind enumerator values; the X86_32TargetInfo constructor in clang/lib/Basic/Targets/X86.h has the following code: 420 // Use fpret for all types. 421 RealTypeUsesObjCFPRet = 422 ((1 << (int)FloatModeKind::Float) | (1 << (int)FloatModeKind::Double) | 423 (1 << (int)FloatModeKind::LongDouble)); NoFloat is a special case for which a mask bit is not needed. I think this code is correct as is, but Aaron's comment suggests some clarification would be useful. Perhaps the data member should be renamed to something like RealTypeUsesObjCFPRetMask (though I suspect a better name can be found). | |
884 | Yes, this should assert if NoFloat is passed. I agree adding a message would be helpful. |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | Ahhhh, yeah, I definitely did not pick up the fact that these were used as part of a mask. I'm more used to masks being represented directly in the enumeration itself, e.g., enum Whatever { FirstThing = 1 << 0, SecondThing = 1 << 1, ThirdThing = 1 << 2 }; These aren't really masks, they're shift amounts to be used to form a mask. Any reason we don't switch to that form to make it more clear (and simplify other code)? Actually, any reason why we don't want to switch the enumeration to use bitmask enums (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h)? |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | I agree using the facilities provided by BitmaskEnum.h would be an improvement. Such a change request is arguably out of scope for this review, but if @jolanta.jensen wants to take that on, then great! I'm ok with the code as is (though a comment or some other explicit indicator that the enumeration and data member are used as a bit mask would be appreciated). |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 | I can happily make the change but I would prefer to carry it out in another patch as using facilities provided by BitmaskEnum is a non-functional change. |
clang/include/clang/Basic/TargetInfo.h | ||
---|---|---|
222–223 |
That's fine by me, thank you! |
The existing enumerators were ordered according to precision. Consider moving Half to before Float if doing so doesn't cause any problems (I would hope there is no dependence on the assigned enumerator values anywhere; if there is, then it would be helpful to add a comment about that here).