llvm dev - May 2012 - [LLVMdev] [PATCH] OpenCL half support

I've got comments on the code change.   The test cases look ok, but I
haven't fully checked the math on the half-values.
I checked with reference to trunk top-of-tree at revision 156617.   I
have not compiled the code.


lib/AsmParser/LLLexer.cpp

   Adds support to parse format: 0xH<hexdigits>
      Tha 0xH format should be described in LangRef.html alongside
0xK<hex> and 0xM<hex>

   The code looks good though.


lib/VMCore/AsmWriter.cpp
   The change updates Printing support for half data type, so it uses
   the new special 0xH<hex> form.

   Declaration of "int shiftcount" should be moved to smallest nesting
   possible, right after "if ( const ConstantFP ..." at line 710

   (The code makes a lot more sense with a good comment on the definition
   of shiftcount.  I prefer to add this comment:
      int shiftcount; // bit position, in the current word, of the
next nibble to print.
   This is a problem with the original code, not the patch.)

   The patch removes half case from the code for single and double.
   So you should remove the "bool isHalf" variable in that spot. (line
720)

   Line 762.  Update the comment
       // Some form of long double.  These appear as a magic letter identifying

   Before:
    // Some form of long double.  These appear as a magic letter identifying
    // the type, then a fixed number of hex digits.
   After:
    // Either half, or some form of long double.
    // These appear as a magic letter identifying the type, then a
    // fixed number of hex digits.

   The rest of this patch to AsmWriter.cpp looks good.



On Fri, Apr 20, 2012 at 7:15 AM, Anton Lokhmotov
<Anton.Lokhmotov at arm.com> wrote:
> Several companies have expressed interest in adding support for the half
> data type.  Some code from our previous patch
> (http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-July/042043.html) made it
> into ToT (at
http://llvm.org/viewvc/llvm-project?view=rev&revision=146786).
>
> Please review further changes required.
>
> Many thanks,
> Anton.
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

Hi David,

Many thanks for the comments!
>       Tha 0xH format should be described in LangRef.html alongside
> 0xK<hex> and 0xM<hex>
Done.
>    Declaration of "int shiftcount" should be moved to smallest
nesting
>    possible, right after "if ( const ConstantFP ..." at line 710
> 
>    (The code makes a lot more sense with a good comment on the
> definition
>    of shiftcount.  I prefer to add this comment:
>       int shiftcount; // bit position, in the current word, of the
> next nibble to print.
>    This is a problem with the original code, not the patch.)
Done.
>    The patch removes half case from the code for single and double.
>    So you should remove the "bool isHalf" variable in that spot.
(line
> 720)
I'm not sure I get it.  This variable is still needed couple of lines below:

      bool isHalf =
&CFP->getValueAPF().getSemantics()==&APFloat::IEEEhalf;
      bool isDouble
&CFP->getValueAPF().getSemantics()==&APFloat::IEEEdouble;
      bool isInf = CFP->getValueAPF().isInfinity();
      bool isNaN = CFP->getValueAPF().isNaN();
      if (!isHalf && !isInf && !isNaN) {
>     // Either half, or some form of long double.
>     // These appear as a magic letter identifying the type, then a
>     // fixed number of hex digits.
Done.

Does it look better to you?

Many thanks,
Anton.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: half-llvm.patch
Type: application/octet-stream
Size: 6477 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120517/9bd258b0/attachment.obj>

looks good here.
> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu]
> On Behalf Of Anton Lokhmotov
> Sent: Thursday, May 17, 2012 4:51 AM
> To: 'David Neto'
> Cc: llvmdev at cs.uiuc.edu
> Subject: Re: [LLVMdev] [PATCH] OpenCL half support
> 
> Hi David,
> 
> Many thanks for the comments!
> 
> >       Tha 0xH format should be described in LangRef.html alongside
> > 0xK<hex> and 0xM<hex>
> Done.
> 
> >    Declaration of "int shiftcount" should be moved to
smallest nesting
> >    possible, right after "if ( const ConstantFP ..." at line
710
> >
> >    (The code makes a lot more sense with a good comment on the
> > definition
> >    of shiftcount.  I prefer to add this comment:
> >       int shiftcount; // bit position, in the current word, of the
> > next nibble to print.
> >    This is a problem with the original code, not the patch.)
> Done.
> 
> >    The patch removes half case from the code for single and double.
> >    So you should remove the "bool isHalf" variable in that
spot. (line
> > 720)
> I'm not sure I get it.  This variable is still needed couple of lines
> below:
> 
>       bool isHalf = &CFP-
> >getValueAPF().getSemantics()==&APFloat::IEEEhalf;
>       bool isDouble >
&CFP->getValueAPF().getSemantics()==&APFloat::IEEEdouble;
>       bool isInf = CFP->getValueAPF().isInfinity();
>       bool isNaN = CFP->getValueAPF().isNaN();
>       if (!isHalf && !isInf && !isNaN) {
> 
> >     // Either half, or some form of long double.
> >     // These appear as a magic letter identifying the type, then a
> >     // fixed number of hex digits.
> Done.
> 
> Does it look better to you?
> 
> Many thanks,
> Anton.