[cfe-commits] [llvm-commits] [patch] fp128 sint_to_fp folding fix

[Renato Golin]

On 8 January 2013 14:14, Tim Northover <Tim.Northover at arm.com> wrote:

> I've got some updated patches here that remove any default behaviour; as
> Duncan pointed out it *is* largely inappropriate. The new interface is
>
>     APFloat::APFloat(const fltSemantics&, const APInt&)
>

I agree both alternatives were worse than this. I think it turned out
rather cleaner than I expected.


On the whole it turned out rather neatly. When needed, the fltSemantics were
> usually easy to come by, even though there's a redundancy in APInt size and
> semantics for the smaller cases.
>

It looks better now, indeed, including the removal of the redundant
static EVTToAPFloatSemantics().

The LLVM patch looks good to me.

On the Clang patch:

+enum APFloatSemantics {
+  IEEEhalf,
+  IEEEsingle,
+  IEEEdouble,
+  x87DoubleExtended,
+  IEEEquad,
+  PPCDoubleDouble
+};

I not sure what would be the best approach. It seems redundant and error
prone to have this here...

Having the same enum on APFloat.h would only be marginally better, though.

-  return llvm::APFloat(ReadAPInt(Record, Idx));
+  APFloatSemantics Semantics =
static_cast<APFloatSemantics>(Record[Idx++]);

Maybe this is better to be in a function itself, say ReadAPFloat().

How does the producer of Record knows about that enum in the first place?
Seems odd that you just magically assumed both parts are agreeing to the
same interface when the produce is not using it... Or I got it completely
wrong, quite likely... ;)

cheers,
--renato
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130108/dfb70699/attachment.html>