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

[Quentin Colombet]

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


+    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> wrote:

>> 
>> On Feb 9, 2015, at 1:47 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> 
>> On Feb 9, 2015, at 1:44 PM, Fiona Glaser <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/f1a4f7d7/attachment.html>