[Clang] Allow 'Complex float __attribute__((mode(HC)))'

[jolanta.jensen]

Summary

Adding half float to types that can be represented by attribute((mode(xx))).
Original implementation authored by George Steed.

[mgabka]

clang/test/CodeGen/aarch64-attr-mode-complex.c

12

Hi Jolanta,
I the tests from this file, I don't think these check lines are necessary, if it was me I would rather check only the type of the argument passed into the function and also the return type.
If you did it like that you could also remove the -Ofast from the RUN lines, as it does not have impact on the functionality you are testing here.

clang/test/Sema/attr-mode.c

40

shouldn't we have here and in the line below : "// expected-no-diagnostics" ?

[jolanta.jensen]

Addressing review comments.

[jolanta.jensen]

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.

[aaron.ballman]

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

3–4

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.

[tahonermann]

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

[tahonermann]

clang/include/clang/Basic/TargetInfo.h

60

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

[jolanta.jensen]

clang/include/clang/Basic/TargetInfo.h

60

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
define void @t0() #0 {
entry:

%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
define void @t0() #0 {
entry:

%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
declare i8* @objc_msgSend(i8*, i8*, ...) #1

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" }
attributes #1 = { nonlazybind }

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
index 57bf4278251a..32e163b9cf38 100644

• a/clang/include/clang/Basic/TargetInfo.h

+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -52,12 +52,12 @@ namespace Builtin { struct Info; }

enum class FloatModeKind {

NoFloat = 255,

• Float = 0,

+ Half = 0,
+ Float,

Double,
LongDouble,
Float128,

• Ibm128,
• Half

+ Ibm128
};

/// Fields controlling how types are laid out in memory; these may need to
@@ -219,7 +219,7 @@ protected:

mutable VersionTuple PlatformMinVersion;

unsigned HasAlignMac68kSupport : 1;

• unsigned RealTypeUsesObjCFPRet : 3;

+ 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?

[jolanta.jensen]

Addressing review comments:

1. Added the test for mode(HF)
2. Moved Half before Float in FloatModeKind enum class to preserve precision ordering

[tahonermann]

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:

1. Add a Last = Ibm128 enumerator to FloatModeKind.
2. Add an assert in useObjCFPRetForRealType() to ensure that T <= FloatModeKind::Last.
3. Add a setUseObjCFPRetForRealType() function with a matching assert to be used to set bits in RealTypeUsesObjCFPRet.
4. Replace the explicit modifications of RealTypeUsesObjCFPRet in X86_32TargetInfo and X86_64TargetInfo with calls to the new setUseObjCFPRetForRealType() function.

[jolanta.jensen]

Removing the implicit dependency on the FloatModeKind enumerator values.

[tahonermann]

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;

[jolanta.jensen]

Correcting my buggy computation of RealTypeUsesObjCFPRet bit-field.

[jolanta.jensen]

clang/include/clang/Basic/TargetInfo.h

222–223

Sorry. I mixed things up.

[tahonermann]

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.

[aaron.ballman]

clang/include/clang/Basic/TargetInfo.h

222–223

I think what we want is:
unsigned RealTypeUsesObjCFPRet : (int)FloatModeKind::Last + 1;

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.

[tahonermann]

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.

[aaron.ballman]

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

[tahonermann]

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

[jolanta.jensen]

Updated Release Notes.
Renamed RealTypeUsesObjCFPRet to RealTypeUsesObjCFPRetMask.

[jolanta.jensen]

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.

[aaron.ballman]

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.

That's fine by me, thank you!

[aaron.ballman]

LGTM!

[tahonermann]

Looks good to me! Thanks Jolanta!

clang/include/clang/Basic/TargetInfo.h

 namespace Builtin { struct Info; }
 
 enum class FloatModeKind {
   NoFloat = 255,
   Float = 0,
   Double,
   LongDouble,
   Float128,
-  Ibm128
+  Ibm128,
+  Half
 };
 
 /// Fields controlling how types are laid out in memory; these may need to
 /// be copied for targets like AMDGPU that base their ABIs on an auxiliary
 /// CPU target.
 struct TransferrableTargetInfo {
   unsigned char PointerWidth, PointerAlign;
   unsigned char BoolWidth, BoolAlign;
protected:
   TargetCXXABI TheCXXABI;
   const LangASMap *AddrSpaceMap;
   unsigned ProgramAddrSpace;
 
   mutable StringRef PlatformName;
   mutable VersionTuple PlatformMinVersion;
 
   unsigned HasAlignMac68kSupport : 1;
   unsigned RealTypeUsesObjCFPRet : 3;
   unsigned ComplexLongDoubleUsesFP2Ret : 1;
 
   unsigned HasBuiltinMSVaList : 1;
 
   unsigned IsRenderScriptTarget : 1;
 
   unsigned HasAArch64SVETypes : 1;
 
   unsigned HasRISCVVTypes : 1;
public:
   /// integer type enum.
   ///
   /// For example, SignedLong -> "l".
   static const char *getTypeFormatModifier(IntType T);
 
   /// Check whether the given real type should use the "fpret" flavor of
   /// Objective-C message passing on this target.
   bool useObjCFPRetForRealType(FloatModeKind T) const {
     return RealTypeUsesObjCFPRet & (1 << (int)T);
   }
 
   /// Check whether _Complex long double should use the "fp2ret" flavor
   /// of Objective-C message passing on this target.
   bool useObjCFP2RetForComplexLongDouble() const {
     return ComplexLongDoubleUsesFP2Ret;
   }
 

clang/lib/AST/ASTContext.cpp

 /// getRealTypeForBitwidth -
 /// sets floating point QualTy according to specified bitwidth.
 /// Returns empty type if there is no appropriate target types.
 QualType ASTContext::getRealTypeForBitwidth(unsigned DestWidth,
                                             FloatModeKind ExplicitType) const {
   FloatModeKind Ty =
       getTargetInfo().getRealTypeByWidth(DestWidth, ExplicitType);
   switch (Ty) {
+  case FloatModeKind::Half:
+    return HalfTy;
   case FloatModeKind::Float:
     return FloatTy;
   case FloatModeKind::Double:
     return DoubleTy;
   case FloatModeKind::LongDouble:
     return LongDoubleTy;
   case FloatModeKind::Float128:
     return Float128Ty;

clang/lib/Basic/TargetInfo.cpp

if (getLongWidth() >= BitWidth)
     return IsSigned ? SignedLong : UnsignedLong;
   if (getLongLongWidth() >= BitWidth)
     return IsSigned ? SignedLongLong : UnsignedLongLong;
   return NoInt;
 }
 
 FloatModeKind TargetInfo::getRealTypeByWidth(unsigned BitWidth,
                                              FloatModeKind ExplicitType) const {
+  if (getHalfWidth() == BitWidth)
+    return FloatModeKind::Half;
   if (getFloatWidth() == BitWidth)
     return FloatModeKind::Float;
   if (getDoubleWidth() == BitWidth)
     return FloatModeKind::Double;
 
   switch (BitWidth) {
   case 96:
     if (&getLongDoubleFormat() == &llvm::APFloat::x87DoubleExtended())

clang/test/CodeGen/aarch64-attr-mode-complex.c

+// RUN: %clang_cc1 -triple arm64-none-linux-gnu -emit-llvm %s -o - | FileCheck %s
+
+typedef _Complex float c16a __attribute((mode(HC)));
+typedef _Complex double c16b __attribute((mode(HC)));
+typedef _Complex float c32a __attribute((mode(SC)));
+typedef _Complex double c32b __attribute((mode(SC)));
+typedef _Complex float c64a __attribute((mode(DC)));
+typedef _Complex double c64b __attribute((mode(DC)));
+
+// CHECK: define{{.*}} { half, half } @c16_test([2 x half] noundef {{.*}}
+// CHECK:   ret { half, half } {{.*}}
+c16b c16_test(c16a x) {
+  return x + x;
+}
+
+// CHECK: define{{.*}} { float, float } @c32_test([2 x float] noundef {{.*}})
+// CHECK:   ret { float, float } {{.*}}
+c32b c32_test(c32a x) {
+  return x + x;
+}
+
+// CHECK: define{{.*}} { double, double } @c64_test([2 x double] noundef {{.*}})
+// CHECK:   ret { double, double } {{.*}}
+c64b c64_test(c64a x) {
+  return x + x;
+}

clang/test/Sema/attr-mode-vector-types.c

 // expected-warning@-1{{specifying vector types with the 'mode' attribute is deprecated; use the 'vector_size' attribute instead}}
 typedef float v4sf __attribute__((mode(SF)))  __attribute__ ((vector_size(16)));
 
 // Incorrect cases.
 typedef float __attribute__((mode(QC))) __attribute__((vector_size(256))) vec_t8;
 // expected-error@-1{{unsupported machine mode 'QC'}}
 // expected-error@-2{{type of machine mode does not match type of base type}}
 typedef _Complex float __attribute__((mode(HC))) __attribute__((vector_size(256))) vec_t9;
-// expected-error@-1{{unsupported machine mode 'HC'}}
-// expected-error@-2{{invalid vector element type '_Complex float'}}
+// expected-error@-1{{invalid vector element type '_Complex float'}}
 typedef int __attribute__((mode(SC))) __attribute__((vector_size(256))) vec_t10;
 // expected-error@-1{{type of machine mode does not match type of base type}}
 // expected-error@-2{{type of machine mode does not support base vector types}}
 typedef float __attribute__((mode(DC))) __attribute__((vector_size(256))) vec_t11;
 // expected-error@-1{{type of machine mode does not match type of base type}}
 // expected-error@-2{{type of machine mode does not support base vector types}}
 typedef _Complex float __attribute__((mode(XC))) __attribute__((vector_size(256))) vec_t12;
 // expected-error@-1{{invalid vector element type '_Complex float'}}
 typedef int __attribute__((mode(V3QI))) v3qi;
 // expected-error@-1{{unknown machine mode 'V3QI'}}

clang/test/Sema/attr-mode.c

 
 typedef unsigned unwind_word __attribute((mode(unwind_word)));
 
 int **__attribute((mode(QI)))* i32;  // expected-error{{mode attribute}}
 
 __attribute__((mode(QI))) int invalid_func(void) { return 1; } // expected-error{{'mode' attribute only applies to variables, enums, typedefs, and non-static data members}}
 enum invalid_enum { A1 __attribute__((mode(QI))) }; // expected-error{{'mode' attribute only applies to}}
 
+typedef _Complex float c16a __attribute((mode(HC)));
+int c16a_test[sizeof(c16a) == 4 ? 1 : -1];
+typedef _Complex double c16b __attribute((mode(HC)));
+int c16b_test[sizeof(c16b) == 4 ? 1 : -1];
+
 typedef _Complex double c32 __attribute((mode(SC)));
 int c32_test[sizeof(c32) == 8 ? 1 : -1];
 typedef _Complex float c64 __attribute((mode(DC)));
 
 #if !defined(__ppc__) && !defined(__mips__) // Note, 'XC' mode is illegal for PPC64 and MIPS machines.
 typedef _Complex float c80 __attribute((mode(XC)));
 #endif