[cfe-dev] [OpenCL patch] Half type as native when OpenCL's cl_khr_fp16 extension is enabled

[John Garvin]

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