Hi! On 11/18/21 10:22 AM, Segher Boessenkool wrote: > On Thu, Nov 18, 2021 at 10:09:53AM -0600, Bill Schmidt wrote: >> Hi! This patch is broken out from the test case patch for the new builtins >> support. >> >> The old builtins code performs gimple folding on 128-bit compares. This >> results in correct but very inefficient code. (I suspect we may be >> missing some optab entries, misleading gimple into 64-bit emulation.) > It is sub-optimal if Gimple ever does this: it is better done later, at > expand time. > >> In >> the new support, I disabled this gimple folding, which results in us >> directly generating the 128-bit comparison instructions. This patch >> adjusts the scan-assembler-times counts for those instructions. >> >> I've opened PR103316 to track this. > Thanks. > > So when the generic code wisens up this testcase will still pass? But > you do then need to re-introduce the folding here?
Right. If we fix the generic code, the test will still pass. We need to re-introduce the folding to leverage it, and will only do that if the test still passes. We always want these single-instruction comparisons to fall out for simple tests like these. > >> gcc/testsuite/ >> * gcc.target/powerpc/int_128bit-runnable.c: Adjust instruction >> counts since we do better by not gimple-folding some builtins. > Wrap later please? 80 chars is fine, 79 chars is fine, 10 chars or 70 > chars is not :-( > > (Not that it matters much *here* of course; it just annoys me A slight argument in favor of earlier wrapping: With Git, the ChangeLog entries in the commit messages get indented, so wrapping a little earlier makes those much easier to read. That's why I started reducing the length of my entries a little. Not a big deal either way, but it's really noticeable in git log output. > > Also, s/ since.*/./ please. Changelogs say what changed, not why, and > the "why" you say here is only half of the story, pretty misleading for > future archaeologists. Good call. > >> --- a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c >> +++ b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c >> @@ -11,9 +11,9 @@ >> /* { dg-final { scan-assembler-times {\mvrlq\M} 2 } } */ >> /* { dg-final { scan-assembler-times {\mvrlqnm\M} 2 } } */ >> /* { dg-final { scan-assembler-times {\mvrlqmi\M} 2 } } */ >> -/* { dg-final { scan-assembler-times {\mvcmpequq\M} 16 } } */ >> -/* { dg-final { scan-assembler-times {\mvcmpgtsq\M} 16 } } */ >> -/* { dg-final { scan-assembler-times {\mvcmpgtuq\M} 16 } } */ >> +/* { dg-final { scan-assembler-times {\mvcmpequq\M} 24 } } */ >> +/* { dg-final { scan-assembler-times {\mvcmpgtsq\M} 26 } } */ >> +/* { dg-final { scan-assembler-times {\mvcmpgtuq\M} 26 } } */ >> /* { dg-final { scan-assembler-times {\mvmuloud\M} 1 } } */ >> /* { dg-final { scan-assembler-times {\mvmulesd\M} 1 } } */ >> /* { dg-final { scan-assembler-times {\mvmulosd\M} 1 } } */ > If you think it actually generates better code now, and this is expected > code, then okay for trunk. Thanks! Thanks very much for the review! Bill > > > Segher