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

[John Garvin]

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
>