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

[Fiona Glaser]

> On Feb 11, 2015, at 8:49 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
> 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 think you might be right here; gcc seems to do this optimization, even if it superficially seems a little bit of a scary (ab)use of undefined behavior.

> 
> 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())

Hmm, okay, if you think it would be clearer that way...

> 
>  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.

Okay, will do.

> 
> +    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?

I don’t understand what the ReplaceInstUsesWith is for; similar codepaths in the same file don’t seem to do that, and I don’t know enough about InstCombine to know why. I was guessing it was because they needed to refer to an existing node instead of a new node.

Fiona