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

Mehdi Amini mehdi.amini at apple.com
Wed Feb 11 12:24:29 PST 2015


CC: Owen for advising about the DAG part at the end.


> On Feb 11, 2015, at 11:09 AM, Fiona Glaser <fglaser at apple.com> wrote:
> 
> Okay, I’ve implemented a cleaner and nicer InstCombine per suggestions.

Great refactoring in FoldItoFPtoI()! 
Can you add a comment above the function?


+  Instruction *OpI = dyn_cast<Instruction>(FI.getOperand(0));

I don’t think you need a dyn_cast here (you need check the result of the dyn_cast). You should be able to do something:

 if (!isa<UIToFPInst>(FI.getOperand(0)) && !isa<SIToFPInst>(FI.getOperand(0)))
   return nullptr;
 Instruction *OpI = cast<Instruction>();


+  bool IsOutputSigned = isa<FPToSIInst>(FI);

You have the information statically at the call site, it could be a function argument instead of a dynamic check. (If you think it is more clear this way, keep it)


+ int InputSize = (int)SrcTy->getScalarSizeInBits()-IsInputSigned;
+ int OutputSize = (int)FITy->getScalarSizeInBits()-IsOutputSigned;

spacing around the minus.


+        return new SExtInst(OpI->getOperand(0), FITy);
+      else

No else after return :)


Very Minor: OpI->getOperand(0) is used 6 times, might worth using a named variable for it.



There is a few edge cases that are not covered in the tests. I can remove the "-IsInputSigned;” and "-IsOutputSigned;” in the code below without making the validation failing.

+ int InputSize = (int)SrcTy->getScalarSizeInBits()-IsInputSigned;
+ int OutputSize = (int)FITy->getScalarSizeInBits()-IsOutputSigned;


Here are some relevant tests:


; This can fold because the 25-bit input is signed and we disard the signed bit
; CHECK-LABEL: test16
; CHECK: zext
define i32 @test16(i25 %A) nounwind {
  %B = sitofp i25 %A to float
  %C = fptoui float %B to i32
  ret i32 %C
}

; This can't fold because the 26-bit input won't fit the mantissa
; even after disarding the signed bit
; CHECK-LABEL: test17
; CHECK: sitofp
; CHECK-NEXT: fptoui
define i32 @test17(i26 %A) nounwind {
  %B = sitofp i26 %A to float
  %C = fptoui float %B to i32
  ret i32 %C
}

; This can fold because the 54-bit output is signed and we disard the signed bit
; CHECK-LABEL: test18
; CHECK: trunc
define i54 @test18(i64 %A) nounwind {
  %B = sitofp i64 %A to double
  %C = fptosi double %B to i54
  ret i54 %C
}

; This can't fold because the 55-bit output won't fit the mantissa
; even after disarding the signed bit
; CHECK-LABEL: test19
; CHECK: sitofp
; CHECK-NEXT: fptosi
define i55 @test19(i64 %A) nounwind {
  %B = sitofp i64 %A to double
  %C = fptosi double %B to i55
  ret i55 %C
}


> 
> The two things I remain unsure about:
> 
> 1) is the use of replacealluseswith correct? Should I be using it elsewhere?
> 
> 2) should the DAG combine be enhanced too to match the InstCombine? That is, are we allowed to take advantage of undefined behavior in the DAGCombine, or is that bad because the target may shuffle around the DAG in ways that relies on target-specific behavior?

I assume that if a target wants “target specific” behavior, they have to use their own ISD node.

Thanks,

Mehdi



More information about the llvm-commits mailing list