[X86] CVTTPD2DQ/CVTTPS2DQ intrinsics must not map to ISD::FP_TO_SINT etc.

[RKSimon]

Bugzilla Link	37751
Resolution	FIXED
Resolved on	Jun 14, 2018 07:08
Version	trunk
OS	Windows NT
Blocks	#37146
CC	@adibiagio,@topperc,@efriedma-quic,@filcab,@gregbedwell,@rotateright,@wjristow
Fixed by commit(s)	r334685

Extended Description

Now that we have FP_TO_*INT combines in DAGCombine (rL330437 for rounding etc.), we can no longer safely map x86 f2i intrinsics to generic types:

X86_INTRINSIC_DATA(avx_cvtt_ps2dq_256,INTR_TYPE_1OP, ISD::FP_TO_SINT, 0)

as this will lose the intrinsics' special case handling for out of range values.

All these values need mapping to the X86ISD equivalents (CVTTP2SI/CVTTP2UI) instead and suitable constant folding support adding so we don't lose any functionality.

[RKSimon]

assigned to @rotateright

[gregbedwell]

For fun, here's our C++-level reproducer for a miscompile courtesy of one of our random test generators:

// =====================================================================
// 1486483810

#include <stdio.h>
#include <string.h>
#include <x86intrin.h>

int main() {
float floats[8] = {-2281655195662388218891860000.f,
-594046087145809546197700000000.f,
-154621041715724213339000000000000.f,
-40234568991263022810438000000000000.f,
-10466854733644965633490000000000000000.f,
0.f,
0.f,
0.f};
__m256 xx;
memcpy(&xx, &floats[0], sizeof(__m256));

__m256i yy = _mm256_cvttps_epi32(xx);
__m256 zz = _mm256_cvtepi32_ps(yy);

float floats2[8];
memcpy(&floats2[0], &zz, sizeof(__m256));
for (int i = 0; i < 8; ++i)
printf("result: %d: %f -> %f\n", i, floats[i], floats2[i]);
}
// =====================================================================

clang -O0:

result: 0: -2281655195662388218891860000.000000 -> -2147483648.000000
result: 1: -594046087145809546197700000000.000000 -> -2147483648.000000
result: 2: -154621041715724213339000000000000.000000 -> -2147483648.000000
result: 3: -40234568991263022810438000000000000.000000 -> -2147483648.000000
result: 4: -10466854733644965633490000000000000000.000000 -> -2147483648.000000
result: 5: 0.000000 -> 0.000000
result: 6: 0.000000 -> 0.000000
result: 7: 0.000000 -> 0.000000

clang O2:

result: 0: -2281655195662388218891860000.000000 -> -2281655195662388218891860000.000000
result: 1: -594046087145809546197700000000.000000 -> -594046087145809546197700000000.000000
result: 2: -154621041715724213339000000000000.000000 -> -154621041715724213339000000000000.000000
result: 3: -40234568991263022810438000000000000.000000 -> -40234568991263022810438000000000000.000000
result: 4: -10466854733644965633490000000000000000.000000 -> -10466854733644965633490000000000000000.000000
result: 5: 0.000000 -> 0.000000
result: 6: 0.000000 -> 0.000000
result: 7: 0.000000 -> 0.000000

[rotateright]

We can't just change the mapping from IR intrinsic to X86 node because we don't have the td patterns to lower those.

Oddly, we already have the correct mapping/patterns for:
X86_INTRINSIC_DATA(sse2_cvttpd2dq, INTR_TYPE_1OP, X86ISD::CVTTP2SI, 0),

but not these:
X86_INTRINSIC_DATA(avx_cvtt_pd2dq_256,INTR_TYPE_1OP, ISD::FP_TO_SINT, 0),
X86_INTRINSIC_DATA(avx_cvtt_ps2dq_256,INTR_TYPE_1OP, ISD::FP_TO_SINT, 0),
X86_INTRINSIC_DATA(sse2_cvttps2dq, INTR_TYPE_1OP, ISD::FP_TO_SINT, 0),

[RKSimon]

We can't just change the mapping from IR intrinsic to X86 node because we
don't have the td patterns to lower those.

Oddly, we already have the correct mapping/patterns for:
X86_INTRINSIC_DATA(sse2_cvttpd2dq, INTR_TYPE_1OP, X86ISD::CVTTP2SI, 0),

That's because CVTTP2SI is allowed to be a bit more permissive than FP_TO_SINT, allowing the necessary partial subvectors to be read/written for different sized results, plus we assume that any remaining upper written bits are zero.

Adding extra patterns shouldn't be a big issue tbh, although we should probably use CVTTP2SI in the defs and move the FP_TO_SINT to extra lowering patterns.

[rotateright]

Adding extra patterns shouldn't be a big issue tbh, although we should
probably use CVTTP2SI in the defs and move the FP_TO_SINT to extra lowering
patterns.

Ah, ok - I have a draft in progress, and I wasn't sure what the preference on the order/structure was. Let me update that. There's another question about what we should do with AVX512, but we can view and and sort that out once I post a patch.

[rotateright]

https://reviews.llvm.org/D47993

[RKSimon]

The mappings were updated at rL334685.

[rotateright]

Anything left to do for this report, or close and continue in bug 37798?

[RKSimon]

Anything left to do for this report, or close and continue in bug 37798?

Resolving, it now passes the fuzz test thanks. Cheers.

[RKSimon]

mentioned in issue #37146