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

Thu Aug 29 09:25:33 PDT 2019

lebedev.ri added a comment.

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

> Hello. Sorry for the delay I missed your earlier email and wanted to grab some more results.

No problem and thank you for looking into this!

> This is what it looks like on an Cortex-M0+:
> 
>   cmsisdsp/BasicMath/arm_abs_q15                         61280.3209   51891.8918   -15.32
>   cmsisdsp/BasicMath/arm_abs_q31                         96690.6474   61414.7322   -36.48
>   cmsisdsp/BasicMath/arm_abs_q7                          65869.4373   56112.2244   -14.81
>   cmsisdsp/BasicMath/arm_mult_q15                        49288.5433   42873.5485   -13.01
>   cmsisdsp/BasicMath/arm_mult_q7                         51843.8512   41171.4250   -20.58
>   cmsisdsp/ComplexMath/arm_cmplx_mult_real_q15           33015.6234   27529.7009   -16.61
>   cmsisdsp/Filtering/arm_fir_lattice_q15                 9564.20255   9130.68289   -4.53
>   cmsisdsp/Transform/arm_cfft_q15                        3854.80244   3656.58566   -5.14
>   cmsisdsp/Transform/arm_rfft_q15                        5410.24904   5219.46206   -3.52
>    
>   cmsisdsp/BasicMath                                     40822.2401   39464.0989   -3.32
>   cmsisdsp/ComplexMath                                   12243.6404   12120.6590   -1.00
>   cmsisdsp/Filtering                                     1094.25278   1092.98454   -0.11
>   cmsisdsp/Transform                                     1805.07193   1778.58598   -1.46
>   total                                                  2450.53110   2433.22327   -0.70 
>    
> 
> The first two columns mean very little, but are some measure of items/sec/Mhz (higher is better). The third column is percentage change. The bottom results are geomeans. Any results that didn't change are omitted.

So correct me please if i'm misunderstanding this - on your benchmarks this does not appear to have any measurable regressions?

> Those results are a load better than they were originally. They are still not great, but they are on a CPU/test suite combo that isn't the most interesting combo. This doesn't come up on other, more dsp like cpus, and there are some AArch64 codesize improvements for this patch too.
> 
> But equally I don't think that a lot of these regressions were necessarily about the cpu/architecture used, but about the code they are running.

> You may find that similar problems come up elsewhere from other peoples code,

Yeah, i suspect this will shake loose (temporarily regress) a few more patterns elsewhere.

> but we have no problem with this going ahead.

Great!

By this point, does anyone else have any concerns here?
Anyone feel like reviewing/stamping, or indicating some further steps that must be taken first?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65148