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

[Kevin P. Neal via Phabricator via llvm-commits]

kpn added inline comments.


================
Comment at: test/CodeGen/X86/fp-intrinsics.ll:293
+; CHECK-LABEL: @f20
+; COMMON: cvttsd2si
+define i32 @f20() {
----------------
cameron.mcinally wrote:
> Same here as the vector version below. Do we want the truncating convert? That was surprising to me.
The strict intrinsic results in the same instruction as the regular fptosi instruction. Having them be the same means the mutation from strict to non-strict is working correctly. 

And, yes, rounding towards zero is correct. That's why there's no rounding metadata.


================
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
----------------
cameron.mcinally wrote:
> This surprised me. Should this be the truncating convert? Or should it be vcvtsd2si?
> 
Yes, it should be a truncating conversion.


================
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)
----------------
cameron.mcinally wrote:
> Same question as the scalar versions. Do we want <4 x float> to <4 x i32>/etc casts?
On the odd chance that it may tickle the vector legalizer I'll go ahead and add tests with float.

Same answer as the scalar tests: Rounding towards zero is correct.


================
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)
----------------
cameron.mcinally wrote:
> 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?
This is an opt test. I'm not sure we'd benefit from placing those extra tests here.


https://reviews.llvm.org/D43515