[PATCH] D43515: More math intrinsics for conservative math handling

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added inline comments.


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:293
+; CHECK-LABEL: @f20
+; COMMON: cvttsd2si
+define i32 @f20() {
----------------
Same here as the vector version below. Do we want the truncating convert? That was surprising to me.


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:346
 declare double @llvm.experimental.constrained.fma.f64(double, double, double, metadata, metadata)
+declare i32 @llvm.experimental.constrained.fptosi.i32.f64(double, metadata)
+declare float @llvm.experimental.constrained.fptrunc.f32.f64(double, metadata)
----------------
Do we want unsigned convert tests too? fptoui?

I see that there are SystemZ tests to cover them, so maybe that's sufficient? Just pointing this out so others can see.


================
Comment at: test/CodeGen/X86/vector-constrained-fp-intrinsics.ll:4372
+; HAS-FMA:       # %bb.0: # %entry
+; HAS-FMA-NEXT:    vcvttsd2si {{.*}}(%rip), %rax
+; HAS-FMA-NEXT:    vmovq %rax, %xmm0
----------------
This surprised me. Should this be the truncating convert? Or should it be vcvtsd2si?



================
Comment at: test/CodeGen/X86/vector-constrained-fp-intrinsics.ll:4701
+declare <4 x i32> @llvm.experimental.constrained.fptosi.v4i32.v4f64(<4 x double>, metadata)
+declare <4 x i64> @llvm.experimental.constrained.fptosi.v4i64.v4f64(<4 x double>, metadata)
+declare <4 x float> @llvm.experimental.constrained.fptrunc.v4f32.v4f64(<4 x double>, metadata)
----------------
Same question as the scalar versions. Do we want <4 x float> to <4 x i32>/etc casts?


================
Comment at: test/Feature/fp-intrinsics.ll:309
+declare zeroext i32 @llvm.experimental.constrained.fptoui.f64(double, metadata)
+declare i32 @llvm.experimental.constrained.fptosi.f64(double, metadata)
+declare float @llvm.experimental.constrained.fptrunc.f32(double, metadata)
----------------
I haven't been following along closely, so please forgive if this was already discussed...

Should we have float->i32 casts too? Also double->i64?


================
Comment at: test/Feature/fp-intrinsics.ll:311
+declare float @llvm.experimental.constrained.fptrunc.f32(double, metadata)
+declare double @llvm.experimental.constrained.fpext.f64(double, metadata)
----------------
Should 'fpext.f64' have a float argument instead of a double?


https://reviews.llvm.org/D43515