[cfe-dev] [OpenCL patch] Half type as native when OpenCL's cl_khr_fp16 extension is enabled
Douglas Gregor
dgregor at apple.com
Fri Jun 15 11:26:05 PDT 2012
On Jun 13, 2012, at 8:55 AM, Anton Lokhmotov wrote:
> Hi John,
>
> Sorry I missed your original email. Yes, you are right that the previous
> patch missed the case of array subscripting. Please review the updated
> patch.
>
> Many thanks,
> Anton.
I'm rather unnerved by the prevalence of this pattern in IR generation:
@@ -1363,7 +1363,7 @@ ScalarExprEmitter::EmitScalarPrePostIncDec(const UnaryOperator *E, LValue LV,
// Add the inc/dec to the real part.
llvm::Value *amt;
- if (type->isHalfType()) {
+ if (type->isHalfType() && !CGF.getContext().getLangOpts().OpenCL) {
// Another special case: half FP increment should be done via float
value =
Builder.CreateCall(CGF.CGM.getIntrinsic(llvm::Intrinsic::convert_from_fp16),
Using the OpenCL language option here seems wrong, especially given that we tend to permit OpenCL extensions in other language dialects. I think what you really want to be checking for here is whether the target wants to use native half types vs. faking them with a storage-only int16, per this FIXME you're zapping:
@@ -346,15 +349,15 @@ llvm::Type *CodeGenTypes::ConvertType(QualType T) {
// Half is special: it might be lowered to i16 (and will be storage-only
// type),. or can be represented as a set of native operations.
- // FIXME: Ask target which kind of half FP it prefers (storage only vs
- // native).
- ResultType = llvm::Type::getInt16Ty(getLLVMContext());
+ ResultType = getTypeForFormat(getLLVMContext(),
+ Context.getFloatTypeSemantics(T), Context.getLangOpts().OpenCL);
However, that seems like something that would also impact our semantic analysis. I don't have a solid understanding of the similarities and differences between the OpenCL and ARM NEON's half types, but from your patch it's starting to feel like they are conceptually different types.
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index 68f7469..b576624 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -4020,6 +4020,26 @@ Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
assert(SCSpec != DeclSpec::SCS_typedef &&
"Parser allowed 'typedef' as storage class VarDecl.");
VarDecl::StorageClass SC = StorageClassSpecToVarDeclStorageClass(SCSpec);
+
+ if (getLangOpts().OpenCL && !getOpenCLOptions().cl_khr_fp16)
+ {
+ // If the cl_khr_fp16 extension is disabled, reject declaring variables
+ // of the half and half array type (OpenCL 1.1 6.1.1.1).
+ if (R.getTypePtr()->isHalfType()) {
+ Diag(D.getIdentifierLoc(), diag::err_opencl_half_declaration);
+ D.setInvalidType();
+ SC=SC_None;
+ } else if (R.getTypePtr()->isArrayType()) {
+ CanQual<ArrayType> atp =
+ Context.getCanonicalType(R)->getAs<ArrayType>();
+ if (atp->getElementType()->getTypePtr()->isHalfType()) {
+ Diag(D.getIdentifierLoc(), diag::err_opencl_half_array_declaration);
+ D.setInvalidType();
+ SC=SC_None;
+ }
+ }
+ }
+
There's no need to map down to the canonical type here; isHalfType() will do it for you. This would be much cleaner as:
if (R->isHalfType()) {
// diagnose error
} else if (const ArrayType *ArrayTy = R->getAs<ArrayType>()) {
if (ArrayTy->getElementType()->isHalfType()) {
// diagnose error
}
}
I'm not sure why you're clearing out the storage class (with SC=SC_None), when the storage class has nothing to do with the type here.
Finally, both of these diagnostics should include the half type, e.g.,
declaring half variables is not allowed
should include the type, e.g.,
cannot declare variable of half-precision type 'fp16' (aka 'half')
the same goes for other diagnostics.
- Doug
>> -----Original Message-----
>> From: John Garvin [mailto:jgarvin at apple.com]
>> Sent: 11 June 2012 22:46
>> To: cfe-dev at cs.uiuc.edu; Anton Lokhmotov
>> Subject: Re: [cfe-dev] [OpenCL patch] Half type as native when OpenCL's
>> cl_khr_fp16 extension is enabled
>>
>> Another comment: be sure to check for dereferences of half pointers
>> using array subscripting as well as dereferences using the * operator.
>> For example:
>>
>> half *a;
>> half *b;
>> half *c;
>> ...
>> c[10] = a[10] + b[10]; // this should be an error
>>
>> John
>>
>> On Jun 4, 2012, at 5:35 PM, John Garvin wrote:
>>
>>> Two small comments, if I may:
>>>
>>> @@ -348,13 +351,18 @@ llvm::Type *CodeGenTypes::ConvertType(QualType
>> T) {
>>>
>>> // FIXME: Ask target which kind of half FP it prefers (storage
>> only vs
>>> // native).
>>> - ResultType = llvm::Type::getInt16Ty(getLLVMContext());
>>> + if (Context.getLangOpts().OpenCL)
>>> + ResultType = getTypeForFormat(getLLVMContext(),
>>> + Context.getFloatTypeSemantics(T), /* isOpenCL = */
>> true);
>>> + else
>>> + ResultType = llvm::Type::getInt16Ty(getLLVMContext());
>>> break;
>>> case BuiltinType::Float:
>>> case BuiltinType::Double:
>>>
>>> It seems like the if is unnecessary here, since the logic is already
>> in getTypeForFormat.
>>>
>>> @@ -1763,7 +1763,8 @@ bool Sema::IsFloatingPointPromotion(QualType
>> FromType, QualType ToType) {
>>> return true;
>>>
>>> // Half can be promoted to float.
>>> - if (FromBuiltin->getKind() == BuiltinType::Half &&
>>> + if (!(getLangOpts().OpenCL || getOpenCLOptions().cl_khr_fp16)
>> &&
>>> + FromBuiltin->getKind() == BuiltinType::Half &&
>>> ToBuiltin->getKind() == BuiltinType::Float)
>>> return true;
>>> }
>>>
>>> Did you mean the following? As written, it returns true in the OpenCL
>> case whether or not cl_khr_fp16 is true.
>>>
>>> if ((!getLangOpts().OpenCL || getOpenCLOptions().cl_khr_fp16) &&
>>>
>>> John
>>>
>>> On May 17, 2012, at 5:06 AM, Anton Lokhmotov wrote:
>>>
>>>> Hi David,
>>>>
>>>> Many thanks for your comments.
>>>>
>>>>> Generally the code changes look good. I would like to see some
>> test
>>>>> cases for code generation updates:
>>>>> - half constant generation
>>>>> - conversion to and from half, with other type being "float"
>>>> Please see new test/CodeGenOpenCL/half.cl.
>>>>
>>>>> - ++ and --
>>>> OpenCL 1.1 forbids using the increment and decrement operators on
>>>> floating-point values. We haven't open-sourced the semantic check
>> for this,
>>>> so it's hard for us to test. However, incrementing and decrementing
>> a half
>>>> variable by 2.0f is done in the above test.
>>>>
>>>>> I have one detailed comment:
>>>>> The following part of the patch seems unnecessary.
>>>>> I think the existing code should work because the HalfTypeID should
>> be
>>>>> lower than the other floating types?
>>>>>
>>>>> @@ -641,7 +641,10 @@ Value
>>>>> *ScalarExprEmitter::EmitScalarConversion(Value *Src, QualType
>> SrcType,
>>>>> } else {
>>>>> assert(SrcTy->isFloatingPointTy() && DstTy-
>>> isFloatingPointTy()
>>>>> &&
>>>>> "Unknown real conversion");
>>>>> - if (DstTy->getTypeID() < SrcTy->getTypeID())
>>>>> + int DstId = DstTy->getTypeID();
>>>>> + int SrcId = Src->getType()->getTypeID();
>>>>> + if ((SrcId != llvm::Type::HalfTyID && DstId < SrcId) ||
>>>>> + DstId == llvm::Type::HalfTyID)
>>>>> Res = Builder.CreateFPTrunc(Src, DstTy, "conv");
>>>>> else
>>>>> Res = Builder.CreateFPExt(Src, DstTy, "conv");
>>>> Good point! Removed.
>>>>
>>>> Many thanks,
>>>> Anton.<half-
>> clang.patch>_______________________________________________
>>>> cfe-dev mailing list
>>>> cfe-dev at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>>
>>
>>
> <half-clang.patch>_______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
More information about the cfe-dev
mailing list