I checked more and Clang generates incorrect code even with -O0. In the case of -O0, the problem is different: Clang is doing a pcmpeqd of two different uninitialized memory addresses. This is still incorrect.

It seems to me that Clang is treating _mm_undefined_si128() the same way as uninitialized memory, which is incorrect.

Compare these two functions:

__m128i ReturnOneBits()
{
__m128i dummy = _mm_undefined_si128();
return _mm_cmpeq_epi32(dummy, dummy);
}

bool ReturnTrue()
{
int i;
return i == i;
}

ReturnTrue() always returns false. This is acceptable because the C++ standard states that using the value of uninitialized variables is undefined behavior, so a seemingly impossible result is permissible.

However, for _mm_undefined_si128, it has a behavior defined by Intel. It seems reasonable to me that _mm_undefined_si128 returns some unspecified value, but once _mm_undefined_si128 is "called", the value returned becomes fixed. _mm_undefined_si128 really wouldn't serve a purpose if it were like C++'s uninitialize variable rules, because you could just use an uninitialized __m128i for this.

http://www.playingwithpointers.com/problem-with-undef.html
"an undef value has a potentially new bit pattern of the compiler’s choosing at each use site"

So:

define i1 @​cmp_undef() {
%cmp = icmp eq i32 undef, undef
ret i1 %cmp
}

is not 'true', it is:

$ ./opt -instsimplify -S cmpundef.ll
define i1 @​cmp_undef() {
ret i1 undef
}

I agree that we can solve/work-around this as a front-end bug:
case X86::BI__builtin_ia32_undef128:
case X86::BI__builtin_ia32_undef256:
case X86::BI__builtin_ia32_undef512:
return UndefValue::get(ConvertType(E->getType()));

The suggestion to use setzero seems like a good solution to me, but we'll need to check all of the users of the undef intrinsics to make sure that doesn't cause extra instructions in the asm - or we'd defeat the point of these lovely x86 intrinsics. :)

Another option would be to let the intrinsic survive to the backend and clean it up there. This might have the unintended consequence (like some other LLVM optimization intrinsics) of hindering IR optimizations though.

Btw, in the new poison/undef scheme, we could have _mm_undefined_si128() return freeze poison (freeze is an instruction, so the IR would be %v = freeze poison; ret %v), which is a non-fluctuating but unspecified value.

Sadly, the one-line fix to clang:

Index: lib/CodeGen/CGBuiltin.cpp

--- lib/CodeGen/CGBuiltin.cpp (revision 297300)
+++ lib/CodeGen/CGBuiltin.cpp (working copy)
@@ -7381,7 +7381,10 @@
case X86::BI__builtin_ia32_undef128:
case X86::BI__builtin_ia32_undef256:
case X86::BI__builtin_ia32_undef512:

• return UndefValue::get(ConvertType(E->getType()));

• // The x86 definition of "undef" is not the same as the LLVM definition
• // (SmallPtrSet usage in SROA could introduce non-determinism in Clang #32176 ). We leave the exercise of optimizing away an unnecessary zero
• // constant to the backend.
• return llvm::Constant::getNullValue(ConvertType(E->getType()));

...causes many regression test failures:

Failing Tests (9):
Clang :: CodeGen/avx-builtins.c
Clang :: CodeGen/avx2-builtins.c
Clang :: CodeGen/avx512bw-builtins.c
Clang :: CodeGen/avx512dq-builtins.c
Clang :: CodeGen/avx512f-builtins.c
Clang :: CodeGen/avx512vl-builtins.c
Clang :: CodeGen/avx512vldq-builtins.c
Clang :: CodeGen/sse-builtins.c
Clang :: CodeGen/sse2-builtins.c

...because the x86 headers use _mm_undefined* in the definitions of other intrinsics. Example:

#define _mm_permute_pd(A, C) extension ({
(__m128d)__builtin_shufflevector((__v2df)(__m128d)(A),
(__v2df)_mm_undefined_pd(),
((C) >> 0) & 0x1, ((C) >> 1) & 0x1); })

I think that's a legitimate undef usage there, but couldn't this have used the __builtin_shufflevector version with only 2 params instead?

Ideally, someone would fix those headers...

A lazier alternative would be to replace the undefined intrinsic uses in shufflevector with the first param.

An even lazier alternative would be to just fix all the test failures. The optimizer should easily see that the zeroinitializer is actually not used in cases like the above?

There isn't a form of builtin_shufflevector that takes a single input and a list of indices. There's an exactly 2 argument form that takes a vector mask, but I've never seen that used.

There isn't a form of builtin_shufflevector that takes a single input and a
list of indices. There's an exactly 2 argument form that takes a vector
mask, but I've never seen that used.

Hmmm...there is one regression test for that form, but I'm not sure how to constant initialize that vector mask.

If there's no objection, I'll work on the lazier solution. InstCombine has the smarts to convert a zero operand to an undef in all the cases I see in these tests via SimplifyDemandedVectorElts().

And even if instcombine doesn't run due to -O0, I think SelectionDAGBuilder will turn unused inputs to shuffles to undef as well.

In the longer term would the plan be to try and go back to the 'freeze poison' approach?

Yes - I haven't followed the freeze discussion very closely, but Sanjoy's description sounds like a match for the undef intrinsic needs.

But (correct me if I'm wrong), freeze doesn't exist today, so we need an immediate fix for those undef intrinsics to avoid miscompiles.

If we have a few extra xorps in the final code, it probably doesn't matter much since we tend to sprinkle those around anyway. :)

Yes, sorry I didn't mean to say we shouldn't go ahead now. Just adding a TODO comment about it should be enough, and hopefully someone will remember to change this again...

Patch posted for review:
https://reviews.llvm.org/D30834

This should be fixed after:
https://reviews.llvm.org/rL297588

If you have any cases where you're using mm_undefined* and clang is producing an unnecessary zero-ing instruction (xorps, etc) in optimized assembly, please file a new bug report. Thanks!