[IR] Update to use new shufflevector semantics

[ManuelJBrito]

Summary

Update to use new semantics added in D148637 .

[nikic]

LGTM

[ManuelJBrito]

This change is visible in clang in particular in codegen for the vec_promote builtin.
It now produces poison instead of undef for the undefined elements.
I think this is OK according to IBM's documentation[https://www.ibm.com/docs/en/epfz/5.3?topic=book-semantics]:

When a result or behavior is undefined, it is something you “must not” do.
Use of an undefined feature is likely to produce different results on different > implementations or releases of a PL/I product.
The application program is considered to be in error.

[ManuelJBrito]

Hello Nemanja is the change in codegen for vec_promote OK?

[ManuelJBrito]

Adding some PowerPC folks to review the change in codegen for vec_promote.
AFAICT its OK to have the undefined values be poison.

[qiucf]

Why this changes IR output of following case?

// RUN: clang vecpromote.c -S -o - -O0 -target s390x-linux-gnu -fzvector -emit-llvm
#include <vecintrin.h>

vector int si;
int g;
int i;

void foo() {
  si = vec_promote(g, i);
}

// store <4 x i32> undef, ptr %__vec.i, align 16, !noalias !6  ; this undef becomes poison after this patch
// %2 = load i32, ptr %__scalar.addr.i, align 4, !noalias !6
// %3 = load i32, ptr %__index.addr.i, align 4, !noalias !6
// %4 = load <4 x i32>, ptr %__vec.i, align 16, !noalias !6
// %vecins.i = insertelement <4 x i32> %4, i32 %2, i32 %and.i

I see no PowerPC related case changes. Maybe adding SystemZ folks for comments @uweigand @jonpa

[ManuelJBrito]

In D149548#4405173, @qiucf wrote:

Why this changes IR output of following case?

// RUN: clang vecpromote.c -S -o - -O0 -target s390x-linux-gnu -fzvector -emit-llvm
#include <vecintrin.h>

vector int si;
int g;
int i;

void foo() {
  si = vec_promote(g, i);
}

// store <4 x i32> undef, ptr %__vec.i, align 16, !noalias !6  ; this undef becomes poison after this patch
// %2 = load i32, ptr %__scalar.addr.i, align 4, !noalias !6
// %3 = load i32, ptr %__index.addr.i, align 4, !noalias !6
// %4 = load <4 x i32>, ptr %__vec.i, align 16, !noalias !6
// %vecins.i = insertelement <4 x i32> %4, i32 %2, i32 %and.i

I think it's because vec_promote is implemented in clang using a shufflevector with an undefined mask (-1).
ConstantFold sees that we have a fully undefined mask and it folds the shufflevector to poison.(previously undef)
My concern : is it Ok to have the undefined values of the vector be poison or was the undef used to abide to some specific semantics?
Hope this makes sense.

I see no PowerPC related case changes. Maybe adding SystemZ folks for comments @uweigand @jonpa

My bad I pinged the wrong people, thanks for adding SystemZ folks.

[uweigand]

So the semantics of the vec_promote(a, b) intrinsic is defined as:

Returns a vector with a in element position b. The result is a vector with a in element position b. [...] The other elements of the vector are undefined.

This is currently implemented by using insertvector to place a at position b into a source vector that is undef. The effect should be that when using element b of that vector, we are guaranteed to get a, while using any other element is undefined behavior (just like accessing an uninitialized variable).

To be honest, I'm not sure how exactly the LLVM IR semantics changes here when using a poison source vector instead of undef. I seem to recall that poison propagates over operations - is it true that the result of insertvector on a poison vector is itself poison? If so, then this change would break semantics. However, if the result is a vector with a in element b, and poison only in the other elements, then I guess this would still preserve the expected semantics.

[nlopes]

In D149548#4413598, @uweigand wrote:

So the semantics of the vec_promote(a, b) intrinsic is defined as:

Returns a vector with a in element position b. The result is a vector with a in element position b. [...] The other elements of the vector are undefined.

This is currently implemented by using insertvector to place a at position b into a source vector that is undef. The effect should be that when using element b of that vector, we are guaranteed to get a, while using any other element is undefined behavior (just like accessing an uninitialized variable).

To be honest, I'm not sure how exactly the LLVM IR semantics changes here when using a poison source vector instead of undef. I seem to recall that poison propagates over operations - is it true that the result of insertvector on a poison vector is itself poison? If so, then this change would break semantics. However, if the result is a vector with a in element b, and poison only in the other elements, then I guess this would still preserve the expected semantics.

If a vector is fully initialized with insertvector (i.e., one operation per index), then the value of the base vector is irrelevant. It can be poison.
Poison in vectors is element-wise. <poison, 42> doesn't propagate to <poison, poison>.

[uweigand]

In D149548#4413639, @nlopes wrote:

If a vector is fully initialized with insertvector (i.e., one operation per index), then the value of the base vector is irrelevant. It can be poison.
Poison in vectors is element-wise. <poison, 42> doesn't propagate to <poison, poison>.

OK, that should be fine for SystemZ then. Thanks!

[ManuelJBrito]

In D149548#4413762, @uweigand wrote:

In D149548#4413639, @nlopes wrote:

If a vector is fully initialized with insertvector (i.e., one operation per index), then the value of the base vector is irrelevant. It can be poison.
Poison in vectors is element-wise. <poison, 42> doesn't propagate to <poison, poison>.

OK, that should be fine for SystemZ then. Thanks!

Thanks! I'll move forward then.

llvm/lib/IR/ConstantFold.cpp

 Constant *llvm::ConstantFoldShuffleVectorInstruction(Constant *V1, Constant *V2,
                                                      ArrayRef<int> Mask) {
   auto *V1VTy = cast<VectorType>(V1->getType());
   unsigned MaskNumElts = Mask.size();
   auto MaskEltCount =
       ElementCount::get(MaskNumElts, isa<ScalableVectorType>(V1VTy));
   Type *EltTy = V1VTy->getElementType();
 
-  // Undefined shuffle mask -> undefined value.
+  // Poison shuffle mask -> poison value.
   if (all_of(Mask, [](int Elt) { return Elt == PoisonMaskElem; })) {
-    return UndefValue::get(VectorType::get(EltTy, MaskEltCount));
+    return PoisonValue::get(VectorType::get(EltTy, MaskEltCount));
   }
 
   // If the mask is all zeros this is a splat, no need to go through all
   // elements.
   if (all_of(Mask, [](int Elt) { return Elt == 0; })) {
     Type *Ty = IntegerType::get(V1->getContext(), 32);
     Constant *Elt =
         ConstantExpr::getExtractElement(V1, ConstantInt::get(Ty, 0));

llvm/lib/IR/Instructions.cpp

if (isa<ScalableVectorType>(ResultTy)) {
     Type *VecTy = VectorType::get(Int32Ty, Mask.size(), true);
     if (Mask[0] == 0)
       return Constant::getNullValue(VecTy);
     return UndefValue::get(VecTy);
   }
   SmallVector<Constant *, 16> MaskConst;
   for (int Elem : Mask) {
     if (Elem == PoisonMaskElem)
-      MaskConst.push_back(UndefValue::get(Int32Ty));
+      MaskConst.push_back(PoisonValue::get(Int32Ty));
     else
       MaskConst.push_back(ConstantInt::get(Int32Ty, Elem));
   }
   return ConstantVector::get(MaskConst);
 }
 
 static bool isSingleSourceMaskImpl(ArrayRef<int> Mask, int NumOpElts) {
   assert(!Mask.empty() && "Shuffle mask must contain elements");

llvm/test/Transforms/InstCombine/shufflevec-constant.ll

;
   %tmp3 = bitcast i64 %tmp4 to <2 x float>
   %tmp8 = shufflevector <2 x float> %tmp3, <2 x float> undef, <4 x i32> <i32 0, i32 1, i32 undef, i32 undef>
   %tmp9 = shufflevector <4 x float> zeroinitializer, <4 x float> %tmp8, <4 x i32> <i32 0, i32 1, i32 4, i32 5>
   ret <4 x float> %tmp9
 }
 
 define <4 x i1> @shuffle_undef_fixed() {
 ; CHECK-LABEL: @shuffle_undef_fixed(
-; CHECK-NEXT:    ret <4 x i1> undef
+; CHECK-NEXT:    ret <4 x i1> poison
 ;
   %S = shufflevector <4 x i1> zeroinitializer, <4 x i1> zeroinitializer, <4 x i32> undef
   ret <4 x i1> %S
 }
 
 define <vscale x 4 x i1> @suffle_undef_scalable() {
 ; CHECK-LABEL: @suffle_undef_scalable(
-; CHECK-NEXT:    ret <vscale x 4 x i1> undef
+; CHECK-NEXT:    ret <vscale x 4 x i1> poison
 ;
   %S = shufflevector <vscale x 4 x i1> zeroinitializer, <vscale x 4 x i1> zeroinitializer, <vscale x 4 x i32> undef
   ret <vscale x 4 x i1> %S
 }