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

[Mehdi Amini]

> On Feb 9, 2015, at 2:26 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
> 
> For the instcombine part:
> 
> + if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())
> Shouldn’t we have some checks on the DstTy as well?
> 
> e.g., what happens with:
> sitofp i32 to float
> fptoui float to i8
> 
> We shouldn’t be able to simplify that with a s/zext, do we… Or maybe we can because it is undef??

First not all i32 can fit in a float, it is checked in the outer enclosing test.
Let’s consider instead:

sitofp i16 to float
fptoui float to i8

The result is defined only for input value that will fit in the i8 (Language Ref: " If the value cannot fit in ty2, the results are undefined.).
If that mean that we are allowed to do the transform, then we have to generate a trunc right?

It means also that we don’t really care checking that the input integer size can be represented in the floating point type since the same language reference quote applies, i.e. :

sitofp i128 to float
fptoso float to i128

can be turned into a no-op.

Chandler or someone else familiar with the details of InstCombine might be able to shed some light on what is / isn’t allowed to do when the language ref. talk about “undefined” results. 

— 
Mehdi



> 
> 
> +    if (SrcTy == FITy)
> +      return ReplaceInstUsesWith(FI, OpI->getOperand(0));
> +    else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits())
> +      return new ZExtInst(OpI->getOperand(0), FITy);
> 
> No else after return.
> 
> +    if (SrcTy == FITy)
> +      return ReplaceInstUsesWith(FI, OpI->getOperand(0));
> +    else if (FITy->getScalarSizeInBits() > SrcTy->getScalarSizeInBits()) {
> 
> Ditto.
> 
> +      if (isa<SIToFPInst>(OpI))
> +        return new SExtInst(OpI->getOperand(0), FITy);
> +      else
> +        return new ZExtInst(OpI->getOperand(0), FITy);
> Ditto
> 
> Could you increase the coverage in the tests?
> 1. You miss the input with sitofp.
> 2. Add some tests with multiple uses.
> 
> Also, please check what we actually generate the s/zext or nothing.
> 
> 
> 
> For the DAG combine part:
> 
> +      else if (VT.getScalarSizeInBits() > SrcVT.getScalarSizeInBits())
> +        return DAG.getNode(Signed ? ISD::SIGN_EXTEND : ISD::ZERO_EXTEND,
> +   
> No else after return.
> 
> +    // We can only fold away the float conversion if the input range can be
> +    // represented exactly in the float range.
> +    if (APFloat::semanticsPrecision(sem) >= RequiredPrecision) 
> Same question as the inst combine part, any check on the destination type?
> 
> Same remarks on the tests.
> 
> Q.
> 
> On Feb 9, 2015, at 1:59 PM, Fiona Glaser <fglaser at apple.com <mailto:fglaser at apple.com>> wrote:
> 
>>> 
>>> On Feb 9, 2015, at 1:47 PM, Quentin Colombet <qcolombet at apple.com <mailto:qcolombet at apple.com>> wrote:
>>> 
>>> 
>>> On Feb 9, 2015, at 1:44 PM, Fiona Glaser <fglaser at apple.com <mailto:fglaser at apple.com>> wrote:
>>> 
>>>>>> 
>>>>>> Target-specific expansion or combine can end-up with this situation when it was not catched by ints-combine.
>>>>>> 
>>>>>> So I believe we want it to be done *both* in inst-combine *and* in the DAG.
>>>>> 
>>>>> I agree. That is also what I meant in my initial message.
>>>>> 
>>>>> Cheers,
>>>>> Q.
>>>> 
>>>> Okay, should I merge the two patches then (and maybe include both tests)?
>>> 
>>> Definitely include both tests :), but I’d rather have two different patches.
>> 
>> In that case, are the last versions of each of the patches in this thread (the DAG and instcombine ones) okay?
>> 
>> Fiona
> 

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