[PATCH] D90807: [PowerPC] no readmem property for intrinsics mfvscr

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90807