[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

Wed Aug 10 21:48:06 PDT 2022

aqjune added inline comments.

================
Comment at: clang/test/CodeGen/X86/avx-builtins.c:182
   // CHECK-LABEL: test_mm256_castsi128_si256
-  // CHECK: shufflevector <2 x i64> %{{.*}}, <2 x i64> %{{.*}}, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
+  // CHECK: shufflevector <2 x i64> %{{.*}}, <2 x i64> %{{.*}}, <4 x i32> <i32 0, i32 1, i32 poison, i32 poison>
   return _mm256_castsi128_si256(A);
----------------
aqjune wrote:
> RKSimon wrote:
> > efriedma wrote:
> > > This change might be visible to user code.
> > Yes the length changing casts are worrying me as well - we could update the header to insert zero into the upper elements I suppose, in many cases these would be folded away by AVX ops implicitly zeroing the 128-bits. But we'd definitely have the potential for regressions.
> I quickly skimmed through the headers in clang/lib/Headers and listed the functions calling `__builtin_shufflevector` with at least one -1 mask operand.
> It seems there aren't very many, which is good news; I found 17 functions only ({F19257445}).
> 
> But, correctly fixing these headers seems to require a lot of work.
> Since using the zero vector can cause performance regressions, we need to use a frozen poison (undef) vector to encode a vector having unspecified bits.
> A few months ago, I created D104790 to start using freeze(vector poison) for `mm*_undefined*` intrinsics. However, teaching the existing codebase to successfully deal with the frozen poison vector was a pretty tough job.
> When it comes to fixing the headers, there is even no C intrinsic function that represents a frozen poison vector AFAIK.
> 
> I'll appreciate any idea or help in addressing this issue. :/
Okay, D130339 has finally been merged.

I will make a patch that updates the `mm256_castsi128_si256` and its family functions to emit shufflevector with freeze(poison) operand.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103874/new/

https://reviews.llvm.org/D103874