[PATCH] D111976: [DAG] Create fptosi.sat from clamped fptosi

Mon Oct 25 11:25:23 PDT 2021

efriedma added inline comments.

================
Comment at: llvm/test/CodeGen/AArch64/fpclamptosat_vec.ll:1226
+; CHECK-CVT-LABEL: stest_f16i16_mm:
+; CHECK-CVT:       // %bb.0: // %entry
+; CHECK-CVT-NEXT:    fcvtl2 v1.4s, v0.8h
----------------
This isn't an improvement in the case where we don't have a native half fcvtzs.

================
Comment at: llvm/test/CodeGen/X86/fpclamptosat.ll:1179
 entry:
   %conv = fptosi half %x to i128
   %spec.store.select = call i128 @llvm.smin.i128(i128 %conv, i128 9223372036854775807)
----------------
dmgreen wrote:
> RKSimon wrote:
> > Should we be taking into account the maximum/minimum representable value of the half type as well here?
> Yeah I haven't looked into that at much yet. A fp16 only produces 17 (IIRC) bits of data, so anything larger than that doesn't really make much sense to try and saturate. I kept the test to keep the tests consistent and make sure nothing silly happened.  I wasn't sure if that was best optimized at the llvm level (maybe with some range analysis) or in DAG, but it felt like a different issue that I didn't try and address here.
> 
> Do you think it's worth doing something special with that here? Or removing the tests that don't make a huge amount of sense? Or leaving it as-is?
For actual fptosi.sat, we can't really optimize based on the set of legal finite values because we need to handle +-inf.

Here, the smin/smax are redundant, but if we care, we should probably handle that using ComputeNumSignBits or something like that.  (But we probably don't care.)

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111976/new/

https://reviews.llvm.org/D111976