[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

838

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

838

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], [], []>;
   def int_ppc_altivec_mtvscr : GCCBuiltin<"__builtin_altivec_mtvscr">,
               Intrinsic<[], [llvm_v4i32_ty], []>;
 
 
   // 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]>;
let TargetPrefix = "ppc" in {  // All PPC intrinsics start with "llvm.ppc.".
 
   // Vector Sum Instructions.
   def int_ppc_altivec_vsumsws : GCCBuiltin<"__builtin_altivec_vsumsws">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;
   def int_ppc_altivec_vsum2sws : GCCBuiltin<"__builtin_altivec_vsum2sws">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v4i32_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;
   def int_ppc_altivec_vsum4sbs : GCCBuiltin<"__builtin_altivec_vsum4sbs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;
   def int_ppc_altivec_vsum4shs : GCCBuiltin<"__builtin_altivec_vsum4shs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v8i16_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;
   def int_ppc_altivec_vsum4ubs : GCCBuiltin<"__builtin_altivec_vsum4ubs">,
             Intrinsic<[llvm_v4i32_ty], [llvm_v16i8_ty, llvm_v4i32_ty],
                       [IntrNoMem]>;