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.
Details
Details
- Reviewers
jsji steven.zhang nemanjai - Group Reviewers
Restricted Project - Commits
- rG564066524ad0: [PowerPC] add has side effect for SAT bit clobber intrinsics/instructions
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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, 3Comment Actions
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.
Comment Actions
@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?
| llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
|---|---|---|
| 850 | 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 | |
| llvm/include/llvm/IR/IntrinsicsPowerPC.td | ||
|---|---|---|
| 850 | Fair enough. Updated accordingly. | |
Comment Actions
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.
Comment Actions
Thank you so much for fixing this and I'm sorry I didn't get around to going over it sooner.
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