[PowerPC] add has side effect for SAT bit clobber intrinsics/instructions

[shchenz]

Summary

This patch does two things:

• fix the typo that intrinsic mfvscr should be with no readmem property
• since VSCR is not modeled yet, add has side effect for SAT bit clobber intrinsics/instructions.

[nemanjai]

This change is fine as it is. However, since we don't model the VSCR and we mark saturating intrinsics as having no sideeffects, we can miscompile wrt. this instruction/intrinsic.
Here is an example where we schedule a saturating instruction before the mfvsrc where the user clearly wanted it after:

vector int test(vector int a, vector int b, vector int aa,
                vector signed short *__restrict FromVSCR) {
  vector int c = __builtin_altivec_vsumsws(a, b);

  // Clear the SAT bit in vscr.
  __builtin_altivec_mtvscr((vector signed int){0, 0, 0, 0});
  c += a + b + (vector int)__builtin_altivec_vpkswus(a, b);

  // Check for saturation from first vpkswus.
  *FromVSCR = __builtin_altivec_mfvscr();

  // This vpkswus must not be moved before mfvscr.
  return c + (vector int)__builtin_altivec_vpkswus(b, aa);
}

Only the first vpkswus must be between the mtvscr and mfvscr.
Yet this is what we get from clang:

clang -O3 b.c -S -o - -mcpu=power9
        vsumsws 5, 2, 3
        xxlxor 32, 32, 32
        mtvscr 0
        vadduwm 0, 3, 2
        vpkswus 2, 2, 3
        vpkswus 3, 3, 4 # if this one saturates and the previous one doesn't, we have the wrong result
        mfvscr 1
        stxv 33, 0(9)
        vadduwm 4, 0, 5
        vadduwm 2, 4, 2
        vadduwm 2, 2, 3

[shchenz]

update according comments:
1: fix the typo that intrinsic mfvscr should be with no readmem property
2: add has side effect for SAT register clobber intrinsics.

[shchenz]

@nemanjai Hi, thanks for your comments. I did a further step to add has side effect property for SAT bit clobber intrinsics in the new patch. Could you please help to have another look?

[shchenz]

ping

[shchenz]

gentle ping

[shchenz]

gentle ping

[shchenz]

gentle ping...

[steven.zhang]

llvm/include/llvm/IR/IntrinsicsPowerPC.td

840

I think, we need to set the side effect in the .td to avoid scheduling schedule them.

{ 2447,       3,      1,      4,      439,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2447 = VSUBUWM
{ 2448,       3,      1,      4,      376,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2448 = VSUBUWS
{ 2449,       3,      1,      4,      508,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2449 = VSUM2SWS
{ 2450,       3,      1,      4,      508,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2450 = VSUM4SBS
{ 2451,       3,      1,      4,      508,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2451 = VSUM4SHS
{ 2452,       3,      1,      4,      508,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2452 = VSUM4UBS
{ 2453,       3,      1,      4,      508,    0, 0x28ULL, nullptr, nullptr, OperandInfo85 },  // Inst #2453 = VSUMSWS

[shchenz]

1: add side effect flag for instructions too.

[shchenz]

llvm/include/llvm/IR/IntrinsicsPowerPC.td

840

Fair enough. Updated accordingly.

[steven.zhang]

LGTM now. But hold on for some days to see if @nemanjai has more comments. We need some follow up to revisit the floating point instructions and set the sideeffects flag accordingly before they are modelled in fact.

[nemanjai]

Thank you so much for fixing this and I'm sorry I didn't get around to going over it sooner.

[shchenz]

In D90807#2466923, @nemanjai wrote:

Thank you so much for fixing this and I'm sorry I didn't get around to going over it sooner.

Hah, it doesn't matter. Thanks for your test cases ^^

llvm/include/llvm/IR/IntrinsicsPowerPC.td

def int_ppc_altivec_dstst : GCCBuiltin<"__builtin_altivec_dstst">,
                         []>;
   def int_ppc_altivec_dststt : GCCBuiltin<"__builtin_altivec_dststt">,
               Intrinsic<[],
                         [llvm_ptr_ty, llvm_i32_ty, llvm_i32_ty],
                         []>;
 
   // VSCR access.
   def int_ppc_altivec_mfvscr : GCCBuiltin<"__builtin_altivec_mfvscr">,
-              Intrinsic<[llvm_v8i16_ty], [], [IntrReadMem]>;
+              Intrinsic<[llvm_v8i16_ty], [], [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_mtvscr : GCCBuiltin<"__builtin_altivec_mtvscr">,
-              Intrinsic<[], [llvm_v4i32_ty], []>;
+              Intrinsic<[], [llvm_v4i32_ty], [IntrNoMem, IntrHasSideEffects]>;
 
 
   // Loads.  These don't map directly to GCC builtins because they represent the
   // source address with a single pointer.
   def int_ppc_altivec_lvx :
               Intrinsic<[llvm_v4i32_ty], [llvm_ptr_ty], [IntrReadMem, IntrArgMemOnly]>;
   def int_ppc_altivec_lvxl :
               Intrinsic<[llvm_v4i32_ty], [llvm_ptr_ty], [IntrReadMem, IntrArgMemOnly]>;

 def int_ppc_altivec_vsubsws : PowerPC_Vec_WWW_Intrinsic<"vsubsws">;
 def int_ppc_altivec_vsubcuw : PowerPC_Vec_WWW_Intrinsic<"vsubcuw">;
 def int_ppc_altivec_vsubcuq : PowerPC_Vec_QQQ_Intrinsic<"vsubcuq">;
 
 let TargetPrefix = "ppc" in {  // All PPC intrinsics start with "llvm.ppc.".
   // Saturating multiply-adds.
   def int_ppc_altivec_vmhaddshs : GCCBuiltin<"__builtin_altivec_vmhaddshs">,
               Intrinsic<[llvm_v8i16_ty], [llvm_v8i16_ty,
-                         llvm_v8i16_ty, llvm_v8i16_ty], [IntrNoMem]>;
+                         llvm_v8i16_ty, llvm_v8i16_ty],
+                         [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vmhraddshs : GCCBuiltin<"__builtin_altivec_vmhraddshs">,
               Intrinsic<[llvm_v8i16_ty], [llvm_v8i16_ty,
-                         llvm_v8i16_ty, llvm_v8i16_ty], [IntrNoMem]>;
+                         llvm_v8i16_ty, llvm_v8i16_ty],
+                         [IntrNoMem, IntrHasSideEffects]>;
 
   def int_ppc_altivec_vmaddfp : GCCBuiltin<"__builtin_altivec_vmaddfp">,
               Intrinsic<[llvm_v4f32_ty], [llvm_v4f32_ty,
                          llvm_v4f32_ty, llvm_v4f32_ty], [IntrNoMem]>;
   def int_ppc_altivec_vnmsubfp : GCCBuiltin<"__builtin_altivec_vnmsubfp">,
               Intrinsic<[llvm_v4f32_ty], [llvm_v4f32_ty,
                          llvm_v4f32_ty, llvm_v4f32_ty], [IntrNoMem]>;
 
   // Vector Multiply Sum Instructions.
   def int_ppc_altivec_vmsummbm : GCCBuiltin<"__builtin_altivec_vmsummbm">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v16i8_ty,
                        llvm_v4i32_ty], [IntrNoMem]>;
   def int_ppc_altivec_vmsumshm : GCCBuiltin<"__builtin_altivec_vmsumshm">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v8i16_ty,
                        llvm_v4i32_ty], [IntrNoMem]>;
   def int_ppc_altivec_vmsumshs : GCCBuiltin<"__builtin_altivec_vmsumshs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v8i16_ty,
-                       llvm_v4i32_ty], [IntrNoMem]>;
+                       llvm_v4i32_ty], [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vmsumubm : GCCBuiltin<"__builtin_altivec_vmsumubm">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v16i8_ty,
                        llvm_v4i32_ty], [IntrNoMem]>;
   def int_ppc_altivec_vmsumuhm : GCCBuiltin<"__builtin_altivec_vmsumuhm">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v8i16_ty,
                        llvm_v4i32_ty], [IntrNoMem]>;
   def int_ppc_altivec_vmsumudm : GCCBuiltin<"__builtin_altivec_vmsumudm">,
             Intrinsic<[llvm_v1i128_ty], [llvm_v2i64_ty, llvm_v2i64_ty,
                        llvm_v1i128_ty], [IntrNoMem]>;
   def int_ppc_altivec_vmsumuhs : GCCBuiltin<"__builtin_altivec_vmsumuhs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v8i16_ty,
-                       llvm_v4i32_ty], [IntrNoMem]>;
+                       llvm_v4i32_ty], [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vmsumcud : GCCBuiltin<"__builtin_altivec_vmsumcud">,
             Intrinsic<[llvm_v1i128_ty],
                       [llvm_v2i64_ty, llvm_v2i64_ty, llvm_v1i128_ty], [IntrNoMem]>;
 
   // Vector Multiply Instructions.
   def int_ppc_altivec_vmulesb : GCCBuiltin<"__builtin_altivec_vmulesb">,
           Intrinsic<[llvm_v8i16_ty], [llvm_v16i8_ty, llvm_v16i8_ty],
                     [IntrNoMem]>;
let TargetPrefix = "ppc" in {  // All PPC intrinsics start with "llvm.ppc.".
   def int_ppc_altivec_vmulouw : GCCBuiltin<"__builtin_altivec_vmulouw">,
           Intrinsic<[llvm_v2i64_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
                     [IntrNoMem]>;
   def int_ppc_altivec_vmuloud : PowerPC_Vec_QDD_Intrinsic<"vmuloud">;
 
   // Vector Sum Instructions.
   def int_ppc_altivec_vsumsws : GCCBuiltin<"__builtin_altivec_vsumsws">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vsum2sws : GCCBuiltin<"__builtin_altivec_vsum2sws">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vsum4sbs : GCCBuiltin<"__builtin_altivec_vsum4sbs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vsum4shs : GCCBuiltin<"__builtin_altivec_vsum4shs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vsum4ubs : GCCBuiltin<"__builtin_altivec_vsum4ubs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
 
   // Vector Sign Extension Instructions
   def int_ppc_altivec_vextsb2w : GCCBuiltin<"__builtin_altivec_vextsb2w">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty], [IntrNoMem]>;
   def int_ppc_altivec_vextsb2d : GCCBuiltin<"__builtin_altivec_vextsb2d">,
             Intrinsic<[llvm_v2i64_ty], [llvm_v16i8_ty], [IntrNoMem]>;
   def int_ppc_altivec_vextsh2w : GCCBuiltin<"__builtin_altivec_vextsh2w">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty], [IntrNoMem]>;
def int_ppc_altivec_vmladduhm : GCCBuiltin<"__builtin_altivec_vmladduhm">,
                        llvm_v8i16_ty], [IntrNoMem]>;
 
   // Packs.
   def int_ppc_altivec_vpkpx : GCCBuiltin<"__builtin_altivec_vpkpx">,
             Intrinsic<[llvm_v8i16_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;
   def int_ppc_altivec_vpkshss : GCCBuiltin<"__builtin_altivec_vpkshss">,
             Intrinsic<[llvm_v16i8_ty], [llvm_v8i16_ty, llvm_v8i16_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vpkshus : GCCBuiltin<"__builtin_altivec_vpkshus">,
             Intrinsic<[llvm_v16i8_ty], [llvm_v8i16_ty, llvm_v8i16_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vpkswss : GCCBuiltin<"__builtin_altivec_vpkswss">,
             Intrinsic<[llvm_v8i16_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vpkswus : GCCBuiltin<"__builtin_altivec_vpkswus">,
             Intrinsic<[llvm_v8i16_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vpksdss : GCCBuiltin<"__builtin_altivec_vpksdss">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v2i64_ty, llvm_v2i64_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   def int_ppc_altivec_vpksdus : GCCBuiltin<"__builtin_altivec_vpksdus">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v2i64_ty, llvm_v2i64_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   // vpkuhum is lowered to a shuffle.
   def int_ppc_altivec_vpkuhus : GCCBuiltin<"__builtin_altivec_vpkuhus">,
             Intrinsic<[llvm_v16i8_ty], [llvm_v8i16_ty, llvm_v8i16_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   // vpkuwum is lowered to a shuffle.
   def int_ppc_altivec_vpkuwus : GCCBuiltin<"__builtin_altivec_vpkuwus">,
             Intrinsic<[llvm_v8i16_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
   // vpkudum is lowered to a shuffle.
   def int_ppc_altivec_vpkudus : GCCBuiltin<"__builtin_altivec_vpkudus">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v2i64_ty, llvm_v2i64_ty],
-                      [IntrNoMem]>;
+                      [IntrNoMem, IntrHasSideEffects]>;
 
   // Unpacks.
   def int_ppc_altivec_vupkhpx : GCCBuiltin<"__builtin_altivec_vupkhpx">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty], [IntrNoMem]>;
   def int_ppc_altivec_vupkhsb : GCCBuiltin<"__builtin_altivec_vupkhsb">,
             Intrinsic<[llvm_v8i16_ty], [llvm_v16i8_ty], [IntrNoMem]>;
   def int_ppc_altivec_vupkhsh : GCCBuiltin<"__builtin_altivec_vupkhsh">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty], [IntrNoMem]>;

llvm/test/CodeGen/PowerPC/sat-register-clobber.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -ppc-asm-full-reg-names -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN:   %s -o - -verify-machineinstrs -mcpu=pwr9 | FileCheck %s
+
+define <4 x i32> @test(<4 x i32> %a, <4 x i32> %b, <4 x i32> %aa, <8 x i16>* %FromVSCR) {
+; CHECK-LABEL: test:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    vsumsws v5, v2, v3
+; CHECK-NEXT:    xxlxor vs32, vs32, vs32
+; CHECK-NEXT:    mtvscr v0
+; CHECK-NEXT:    vadduwm v0, v3, v2
+; CHECK-NEXT:    vpkswus v2, v2, v3
+; CHECK-NEXT:    mfvscr v1
+; CHECK-NEXT:    stxv vs33, 0(r9)
+; CHECK-NEXT:    vpkswus v3, v3, v4
+; CHECK-NEXT:    vadduwm v4, v0, v5
+; CHECK-NEXT:    vadduwm v2, v4, v2
+; CHECK-NEXT:    vadduwm v2, v2, v3
+; CHECK-NEXT:    blr
+entry:
+  %0 = tail call <4 x i32> @llvm.ppc.altivec.vsumsws(<4 x i32> %a, <4 x i32> %b)
+  tail call void @llvm.ppc.altivec.mtvscr(<4 x i32> zeroinitializer)
+  %add = add <4 x i32> %b, %a
+  %1 = tail call <8 x i16> @llvm.ppc.altivec.vpkswus(<4 x i32> %a, <4 x i32> %b)
+  %2 = bitcast <8 x i16> %1 to <4 x i32>
+  %3 = tail call <8 x i16> @llvm.ppc.altivec.mfvscr()
+  store <8 x i16> %3, <8 x i16>* %FromVSCR, align 16
+  %4 = tail call <8 x i16> @llvm.ppc.altivec.vpkswus(<4 x i32> %b, <4 x i32> %aa)
+  %5 = bitcast <8 x i16> %4 to <4 x i32>
+  %add1 = add <4 x i32> %add, %0
+  %add2 = add <4 x i32> %add1, %2
+  %add3 = add <4 x i32> %add2, %5
+  ret <4 x i32> %add3
+}
+
+declare <4 x i32> @llvm.ppc.altivec.vsumsws(<4 x i32>, <4 x i32>) #1
+
+declare void @llvm.ppc.altivec.mtvscr(<4 x i32>) #1
+
+declare <8 x i16> @llvm.ppc.altivec.vpkswus(<4 x i32>, <4 x i32>) #1
+
+declare <8 x i16> @llvm.ppc.altivec.mfvscr() #1
+