This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by shchenz on Nov 4 2020, 6:08 PM.

Details

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.

Diff Detail

Event Timeline

shchenz created this revision.Nov 4 2020, 6:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2020, 6:08 PM
shchenz requested review of this revision.Nov 4 2020, 6:08 PM

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 updated this revision to Diff 303112.Nov 5 2020, 6:57 AM
shchenz edited the summary of this revision. (Show Details)

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 retitled this revision from [PowerPC] no readmem property for intrinsics mfvscr to [PowerPC] add has side effect for SAT register clobber intrinsics.Nov 5 2020, 6:58 AM

@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 retitled this revision from [PowerPC] add has side effect for SAT register clobber intrinsics to [PowerPC] add has side effect for SAT bit clobber intrinsics.Nov 5 2020, 7:01 AM
shchenz edited the summary of this revision. (Show Details)

gentle ping

gentle ping

gentle ping...

steven.zhang added inline comments.Dec 14 2020, 6:33 PM
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 updated this revision to Diff 311838.Dec 15 2020, 1:33 AM
shchenz retitled this revision from [PowerPC] add has side effect for SAT bit clobber intrinsics to [PowerPC] add has side effect for SAT bit clobber intrinsics/instructions.
shchenz edited the summary of this revision. (Show Details)

1: add side effect flag for instructions too.

shchenz added inline comments.Dec 15 2020, 1:33 AM
llvm/include/llvm/IR/IntrinsicsPowerPC.td
840

Fair enough. Updated accordingly.

steven.zhang accepted this revision.Dec 15 2020, 7:06 PM

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.

This revision is now accepted and ready to land.Dec 15 2020, 7:06 PM
This revision was landed with ongoing or failed builds.Dec 20 2020, 5:05 PM
This revision was automatically updated to reflect the committed changes.

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

shchenz added a comment.EditedDec 21 2020, 5:53 PM

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 ^^