Passing/returning structures in scalars does not handle poison

[hvdijk]

Originally reduced from an AArch64 miscompile, but I believe it applies to most if not all targets. This example is for x86_64:

typedef short short3 __attribute__((ext_vector_type(3)));
typedef struct { short s[4]; } short4;
short3 f1(short s) {
  return (short3){s, s, s};
}
short4 f2(short s) {
  short3 x = f1(s);
  short4 y;
  __builtin_memcpy(&y, &x, sizeof x);
  return y;
}

Compiling this to LLVM IR produces:

$ bin/clang -emit-llvm -S -O3 -o - test.c
; ModuleID = 'test.c'
source_filename = "test.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable
define dso_local double @f1(i16 noundef signext %s) local_unnamed_addr #0 {
entry:
  %retval.sroa.0 = alloca double, align 8
  %vecinit = insertelement <3 x i16> poison, i16 %s, i64 0
  %vecinit2 = shufflevector <3 x i16> %vecinit, <3 x i16> poison, <3 x i32> zeroinitializer
  store <3 x i16> %vecinit2, ptr %retval.sroa.0, align 8
  %retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0. = load double, ptr %retval.sroa.0, align 8
  ret double %retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable
define dso_local i64 @f2(i16 noundef signext %s) local_unnamed_addr #0 {
entry:
  %vecinit.i = insertelement <3 x i16> poison, i16 %s, i64 0
  %extractVec1 = shufflevector <3 x i16> %vecinit.i, <3 x i16> poison, <4 x i32> <i32 0, i32 0, i32 0, i32 poison>
  %0 = bitcast <4 x i16> %extractVec1 to i64
  ret i64 %0
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3}
!llvm.ident = !{!4}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 8, !"PIC Level", i32 2}
!2 = !{i32 7, !"PIE Level", i32 2}
!3 = !{i32 7, !"uwtable", i32 2}
!4 = !{!"clang version 21.0.0git (https://github.com/llvm/llvm-project 2692c3aa6760f1e4ea015f906926f63ec7dce044)"}

The definition of f2 first does shufflevector, producing %extractVec1 = <4 x i16> <i16 %s, i16 %s, i16 %s, i16 poison>. It then bitcasts the result to i64, and because poison, unlike undef, always applies to all bits, this potentially throws away the well-defined first three elements of the vector-copied-to-array depending on exactly which passes run at exactly which time and how they choose to interpret poison.

In a larger example, I saw this go wrong in a context where SROA chose to split up the individual elements of y and then reconstruct the scalar by doing bitwise operations, where constant folding then kicked in for the constant poison element of the vector, but I believe this heavily reduced example already shows that the Clang-to-LLVM-IR translation is going wrong, even if the LLVM-IR-to-machine-code translation happens to produce the expected machine code so far.

[llvmbot]

@llvm/issue-subscribers-clang-codegen

Author: Harald van Dijk (hvdijk)

Originally reduced from an AArch64 miscompile, but I believe it applies to most if not all targets. This example is for x86_64: ```c typedef short short3 __attribute__((ext_vector_type(3))); typedef struct { short s[4]; } short4; short3 f1(short s) { return (short3){s, s, s}; } short4 f2(short s) { short3 x = f1(s); short4 y; __builtin_memcpy(&y, &x, sizeof x); return y; } ``` Compiling this to LLVM IR produces: ```llvm $ bin/clang -emit-llvm -S -O3 -o - test.c ; ModuleID = 'test.c' source_filename = "test.c" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable
define dso_local double @f1(i16 noundef signext %s) local_unnamed_addr #0 {
entry:
%retval.sroa.0 = alloca double, align 8
%vecinit = insertelement <3 x i16> poison, i16 %s, i64 0
%vecinit2 = shufflevector <3 x i16> %vecinit, <3 x i16> poison, <3 x i32> zeroinitializer
store <3 x i16> %vecinit2, ptr %retval.sroa.0, align 8
%retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0. = load double, ptr %retval.sroa.0, align 8
ret double %retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.retval.sroa.0.0.
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable
define dso_local i64 @f2(i16 noundef signext %s) local_unnamed_addr #0 {
entry:
%vecinit.i = insertelement <3 x i16> poison, i16 %s, i64 0
%extractVec1 = shufflevector <3 x i16> %vecinit.i, <3 x i16> poison, <4 x i32> <i32 0, i32 0, i32 0, i32 poison>
%0 = bitcast <4 x i16> %extractVec1 to i64
ret i64 %0
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

!llvm.module.flags = !{!0, !1, !2, !3}
!llvm.ident = !{!4}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{i32 8, !"PIC Level", i32 2}
!2 = !{i32 7, !"PIE Level", i32 2}
!3 = !{i32 7, !"uwtable", i32 2}
!4 = !{!"clang version 21.0.0git (https://github.com/llvm/llvm-project 2692c3a)"}

The definition of `f2` first does `shufflevector`, producing `%extractVec1 = <4 x i16> <i16 %s, i16 %s, i16 %s, i16 poison>`. It then `bitcast`s the result to `i64`, and because `poison`, unlike `undef`, always applies to all bits, this potentially throws away the well-defined first three elements of the vector-copied-to-array depending on exactly which passes run at exactly which time and how they choose to interpret `poison`.

In a larger example, I saw this go wrong in a context where SROA chose to split up the individual elements of `y` and then reconstruct the scalar by doing bitwise operations, where constant folding then kicked in for the constant `poison` element of the vector, but I believe this heavily reduced example already shows that the Clang-to-LLVM-IR translation is going wrong, even if the LLVM-IR-to-machine-code translation happens to produce the expected machine code so far.
</details>

[efriedma-quic]

%extractVec = shufflevector <3 x i16> %vecinit2, <3 x i16> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 poison> is no good, probably... we end up storing poison to memory, which has bad consequences. Relevant code is CodeGenFunction::EmitStoreOfScalar.

[hvdijk]

I'd like to think the shufflevector in here is fine, it's the bitcast that is a problem: if storing poison to memory is a problem, I think there is a much bigger issue, as there are lots of other ways that poison can end up in vectors that are stored (temporarily) to memory. One that immediately comes to mind is left/right shift with uninitialized vector elements that are intended to later be masked out, possibly producing poison if they happened to result in an out-of-range shift count. If we require users to first call @llvm.umin before doing that just to ensure we have a well-defined result that the user does not care about, we pessimize the generated code for no benefit, we make the hardware perform an extra operation when the hardware was already fine with the original shift.

[nikic]

@hvdijk LLVM currently does not support poison in memory, you will run into exactly the kind of issue you did run into. (I think #143527 is the tracking issue for this now.)

I don't really follow your shift example, isn't that UB? We don't (at least not intentionally) expose any poison semantics in the surface language, it should all be either UB or undef right now.

%extractVec = shufflevector <3 x i16> %vecinit2, <3 x i16> poison, <4 x i32> <i32 0, i32 1, i32 2, i32 poison> is no good, probably... we end up storing poison to memory, which has bad consequences. Relevant code is CodeGenFunction::EmitStoreOfScalar.

This should probably do something like %extractVec = shufflevector <3 x i16> %vecinit2, <3 x i16> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3> instead?

[hvdijk]

Ah, thanks for the reference to that. Since bitcast is defined as "done as if the value had been stored to memory and read back" it seems like it's fundamentally the same problem, just that the reason for why a load had been widened is different.

I don't really follow your shift example, isn't that UB? We don't (at least not intentionally) expose any poison semantics in the surface language, it should all be either UB or undef right now.

OpenCL C, at least, goes out of its way to say that vector division is always defined, and will "result in an unspecified value" in the cases where the scalar result would be undefined (division by zero, or division of the minimal value by -1) for precisely this reason. Although it does not say the same for other operators, I've kind of assumed that the intent has been the same, it's supported by hardware, it's kind of hard to avoid in any sort of predicated vector operations, it's already done by external code, and it appeared to be supported in LLVM by poison results being determined elementwise rather than vectorwide.

If we do need to use undef in order to get reliable results for other operators, I don't mind, but that means there will be follow-up work after this particular instance of poison is addressed.

This should probably do something like %extractVec = shufflevector <3 x i16> %vecinit2, <3 x i16> undef, <4 x i32> <i32 0, i32 1, i32 2, i32 3> instead?

I assume you mean i32 4 at the end there, in which case sure, that would work. Alternatively, what I have seen Clang generate in some slightly different code samples, leave that shufflevector alone, but insert an insertelement of undef in between the shufflevector and the bitcast.

[hvdijk]

Looking at #143527 in more detail, we'd need some way of formalizing either:

• Storing poison to memory stores undef. (To make load widening valid)
• As an exception to the above, loading a previously stored value from memory produces that previous value. (To keep reg2mem valid)

Or:

• Storing poison to memory stores poison in each byte. (To keep reg2mem valid)
• Loading poison from memory results in undef, unless the whole load was poison. (To make load widening valid)

If we had either of those, it does not even matter which one, then the LLVM IR that we have here would actually be fine, the shufflevector would produce a vector with a poison element, but the bitcast would relax that to undef bits in the i64 result.