PR16752[PATCH]: 'mode' attribute for unusual targets doesn't work properly.

[Stepan Dyatkovskiy]

Hi Eli,
Sorry for latency.
As you remember this patch should correct 'mode' attr implementation. 
You proposed to use generic way of type detection for each target: just 
scan for target types and select one with suitable width.

Unfortunately I can't commit patch with generic implementation. I got 
test failure for clang. And I expect more failures for another targets. 
The reason is next.

The target could still keep mode unsupported even if it has type of 
suitable width. I mean the next.
For example this string should cause error for i686 machines (128 bit 
float):
typedef          float f128ibm __attribute__ ((mode (TF)));
But in case of generic approach for all targets it would be passed.
In this case virtual functions allows to implement exceptions.

In this case 'getIntTypeByWidth' may be a bit confusing name. Instead it 
is supposed to return type for some particular mode, not by width. 
Something like getInt/RealTypeForMode(width, sign).
Though, currently I kept the original name.

-Stepan.

Eli Friedman wrote:
> On Sun, Aug 25, 2013 at 9:34 AM, Stepan Dyatkovskiy <stpworld at narod.ru
> <mailto:stpworld at narod.ru>> wrote:
>
>     Hi Eli,
>     Thanks for review!
>
>
>      >
>
>              enum RealType {
>         +    NoFloat = 255,
>                Float = 0,
>                Double,
>         -    LongDouble
>         +    LongDouble,
>              };
>
>         Unless I'm missing something, there isn't any reason not to make
>         NoFloat 0.
>
>     Yes, there is. Since there types could be encoded, and Float is
>     supposed to be encoded as 0. It couses several crashes:
>          Clang :: CodeGenObjC/fpret.m
>          Clang :: CodeGenObjC/metadata-symbols-__64.m
>          Clang :: Sema/attr-mode.c
>
> Oh, okay, that's fine.
>
>
>
>         +  /// \brief Return integer type with specified width.
>         +  virtual IntType getIntTypeByWidth(unsigned BitWidth, bool
>         IsSigned) const;
>         +
>         +  /// \brief Return floating point type with specified width.
>         +  virtual RealType getRealTypeByWidth(unsigned BitWidth) const;
>
>         Instead of making these virtual, would it be possible to make these
>         figure out the appropriate types based on the widths etc. we already
>         compute?  e.g. for the integer types, something like so:
>
>         TargetInfo::IntType TargetInfo::getIntTypeByWidth(
>               unsigned BitWidth, bool IsSigned) const {
>             if (getCharWidth() == BitWidth)
>               return IsSigned ? SignedChar : UnsignedChar;
>             if (getShortWidth() == BitWidth)
>               return IsSigned ? SignedShort : UnsignedShort;
>             if (getIntWidth() == BitWidth)
>               return IsSigned ? SignedInt : UnsignedInt;
>             if (getLongWidth() == BitWidth)
>               return IsSigned ? SignedLong : UnsignedLong;
>             if (getLongLongWidth() == BitWidth)
>               return IsSigned ? SignedLongLong : UnsignedLongLong;
>             return NoInt;
>         }
>
>         I'd prefer to make things as simple as possible for people
>         adding new targets.
>
>     OK. I did it. The reason was to keep current implementation but in
>     better form, so everybody could customize it. Implementation you
>     proposed could work differently, though I can watch for buildbots
>     after commit and reject it if something went wrong.
>
>
>         Sorry it took a little while to get to reviewing this.
>
>     Sorry me to. I was on vocation :-)
>
>
> -    LongDouble
> +    LongDouble,
>     };
>
> I don't recall whether all the compilers we care about accept this
> without complaining; please leave out the extra comma.
>
> Otherwise, LGTM.
>
> -Eli

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ModeAttr-2013-09-03.patch
Type: text/x-diff
Size: 10908 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130903/43da45bc/attachment.patch>