[PATCH] SelectionDAG: fold (fp_to_u/sint (u/sint_to_fp val)) when possible

[Mehdi Amini]

Interesting, you’re right we are using these conversion as cast.
I think the LLVM language reference could be more clear.
In "If the value won’t fit in the floating point type, the results are undefined”, I understand “fit” means “fit exactly”. 
I’d rather read as in the C++ standard "If the value being converted is outside the range of values that can be represented, the behavior is undefined”. They even felt necessary to add that "Loss of precision occurs if the integral value cannot be represented exactly as a value of the floating type"

Anyway, I think we can still exploit the fptosi/fptoui guarantee to replace:

  %fp = sitofp i25 %value to float
  %end = fptosi float %fp to i8

with 
 
  %end = trunc i25 %value to %i8

Don’t you think?

I also haven’t seen any answer to my previous email comments:


+      /* extra bit for sign */
+      (int)SrcTy->getScalarSizeInBits() < OpITy->getFPMantissaWidth()) {

It is present in the existing code, but I’m not sure I understand.
First, I don’t understand the strict < instead of <=.
Then I understand that SIToFPInst has one less bit to be considered because the sign bit wouldn’t be part of the mantissa, and it cannot contribute to a value here (the comment explains why) anyway.
I would have written something like:

   bool isSigned = isa<SIToFPInst>(OpI);
   if ((isa<UIToFPInst>(OpI) || isSigned) &&
       (int)FI.getType()->getScalarSizeInBits() -  isSigned <= /*extra bit for sign */
                    OpI->getType()->getFPMantissaWidth())

 It would be nice to also have the test that covers the edge case, which are I believe: for float i25, i24 and for double i53 and i54.


+    if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())
+      return new ZExtInst(OpI->getOperand(0), FITy);

No need for ReplaceInstUsesWith() in the return statement? As in the previous branch of the if?


Thanks

— 
Mehdi



> On Feb 11, 2015, at 7:52 AM, Fiona Glaser <fglaser at apple.com> wrote:
> 
> Um, this sounds very wrong.
> 
> The mantissa width is all about whether the float conversion causes rounding error, not about whether the value fits in the destination type. gcc doesn’t optimize out (int)(float)(int) for example, because that’s not legal.
> 
> Fiona
> 
>> On Feb 10, 2015, at 9:11 PM, Mehdi Amini <mehdi.amini at apple.com <mailto:mehdi.amini at apple.com>> wrote:
>> 
>> The discussion ended up with: fptoui(), fptosi(), sitofp(), and uitofp() are all undefined if the value cannot fit in the destination type.
>> 
>> So I’d expect we go for the simple solution where we don’t care about getFPMantissaWidth().
>> We only check getScalarSizeInBits() to insert the appropriate trunc or s/zext if necessary.
>> 
>> I assume that ISD::FP_TO_SINT (same with the other) has the same semantic as the IR instruction, and thus is applies to the DAG as well.
>> 
>> — 
>> Mehdi
>> 
>> 
>> 
>>> On Feb 10, 2015, at 6:35 PM, Fiona Glaser <fglaser at apple.com <mailto:fglaser at apple.com>> wrote:
>>> 
>>> Any comments on the rest of this?
>>> 
>>> Fiona
>>> 
>>>> On Feb 9, 2015, at 4:35 PM, Eric Christopher <echristo at gmail.com <mailto:echristo at gmail.com>> wrote:
>>>> 
>>>> Feel free to commit the change to port a test to FileCheck separately (and any time you'd like).
>>>> 
>>>> Thanks!
>>>> 
>>>> -eric
>>>> 
>>>> On Mon Feb 09 2015 at 2:53:45 PM Fiona Glaser <fglaser at apple.com <mailto:fglaser at apple.com>> wrote:
>>>> Ah, sorry, missed the comments about the tests. Updated the DAG patch with edited and improved test and minor code fixes.
>>>> 
>>>> Fiona
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150211/de3101e1/attachment.html>