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

[Anton Lokhmotov]

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.


> -----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
> >
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: half-clang.patch
Type: application/octet-stream
Size: 17305 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120613/af7254a0/attachment.obj>