[PATCH] D65148: [SimplifyCFG] Bump phi-node-folding-threshold from 2 to 3

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D65148#1629010 <https://reviews.llvm.org/D65148#1629010>, @dmgreen wrote:

> > That is a valid transformation: https://rise4fun.com/Alive/bxq
>
> I see, because of the truncs back into an i16. That makes sense.
>
> > So i suppose my question here is - other than this, did you observe any other regressions?
> >  If not, care to tentatively stamp? I will *not* land this patch until after that last needed patch.
>
> It's hard to tell for sure if they are all the same thing. Here's one example of some code if its useful. The code is now smaller, but I think it doesn't like being selects over branches as the chance of executing the branched code is so low:
>
>   void arm_abs_q31(
>       const int * pSrc,
>       int * pDst,
>       unsigned blockSize)
>   {
>     unsigned blkCnt = blockSize >> 2U;
>     while (blkCnt > 0U)
>     {
>       int in = *pSrc++;
>       *pDst++ = (in > 0) ? in : ((in == (~0x7fffffff)) ? 2147483647 : -in);
>       in = *pSrc++;
>       *pDst++ = (in > 0) ? in : ((in == (~0x7fffffff)) ? 2147483647 : -in);
>       in = *pSrc++;
>       *pDst++ = (in > 0) ? in : ((in == (~0x7fffffff)) ? 2147483647 : -in);
>       in = *pSrc++;
>       *pDst++ = (in > 0) ? in : ((in == (~0x7fffffff)) ? 2147483647 : -in);
>       blkCnt--;
>     }
>     blkCnt = blockSize % 0x4U;
>     while (blkCnt > 0U)
>     {
>       int in = *pSrc++;
>       *pDst++ = (in > 0) ? in : ((in == (~0x7fffffff)) ? 2147483647 : -in);
>       blkCnt--;
>     }
>   }
>
>
> That's from the same CMSIS DSP suite again, and only when compiling for v6m. Which might not be the most interesting of suites. I've not seen any significant changes anywhere else.


Oh, nice one. Thank you for taking a look!

This one is different; as per me, it's a true-positive improvement (will add a test).
If this is one-time scalar `abs`, then i suspect either variant is ok, but if it't done in a loop,
i suspect branch-less `abs` will be better overall, unless of course the the negativity check was predictable..

But then if that knowledge was present (PGO, `__builtin_expect()`),
then we will get the branch back in the end: https://godbolt.org/z/3Evpf6
Although that does not happen for ARM, so "you've got a bug": https://godbolt.org/z/91UQRK

TLDR: `arm_abs_q31()` change is not an issue i'm going to look into.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65148