Re: ping x2 [PATCH 0/5] MSP430: Implement macros to describe relative costs of operations

[Jozef Lawrynowicz <jozef.l@mittosystems.com>]

On Fri, Nov 06, 2020 at 01:53:19PM -0700, Jeff Law via Gcc-patches wrote:
> 
> On 9/15/20 2:30 PM, Jozef Lawrynowicz wrote:
> > Ping x2 for below.
> >
> > On Fri, Aug 07, 2020 at 12:02:59PM +0100, Jozef Lawrynowicz wrote:
> >> Pinging for this series of patches.
> >> Attached all patches to this mail with the ammended patch 4 thanks to
> >> Segher's review.
> >>
> >> Thanks,
> >> Jozef
> >>
> >> On Thu, Jul 23, 2020 at 04:43:56PM +0100, Jozef Lawrynowicz wrote:
> >>> The following series of patches for MSP430 implement some of the target
> >>> macros used to determine the relative costs of operations.
> >>>
> >>> To give an indication of the overall effect of these changes on
> >>> codesize, below are some size statistics collected from all the
> >>> executable files from execute.exp that are built at -Os.
> >>> There are around 1470 such tests (depending on the configuration).
> >>>
> >>> The percentage change (((new - old)/old) * 100) in text size is calculated
> >>> for each test and the given metric is applied to that overall set of data.
> >>>
> >>> Configuration | Mean (%) | Median (%) | Delta < 0 (count) | Delta > 0 (count)
> >>> -----------------------------------------------------------------------------
> >>> -mcpu=msp430  |  -2.4    |   -2.7     |      1454         |      17
> >>> -mcpu=msp430x |  -2.3    |   -2.4     |      1460         |      10
> >>> -mlarge       |  -1.7    |   -1.9     |      1412         |      37
> >>>
> >>> Successfully regtested on trunk for msp430-elf, ok to apply?
> >>>
> >>> Jozef Lawrynowicz (5):
> >>>   MSP430: Implement TARGET_MEMORY_MOVE_COST
> >>>   MSP430: Implement TARGET_RTX_COSTS
> >>>   MSP430: Add defaulting to the insn length attribute
> >>>   MSP430: Implement TARGET_INSN_COST
> >>>   MSP430: Skip index-1.c test
> >>>
> >>>  gcc/config/msp430/msp430-protos.h             |   5 +-
> >>>  gcc/config/msp430/msp430.c                    | 867 ++++++++++++++++--
> >>>  gcc/config/msp430/msp430.h                    |  13 +
> >>>  gcc/config/msp430/msp430.md                   | 439 +++++++--
> >>>  gcc/config/msp430/msp430.opt                  |   4 +
> >>>  gcc/config/msp430/predicates.md               |  13 +
> >>>  gcc/testsuite/gcc.c-torture/execute/index-1.c |   2 +
> >>>  7 files changed, 1206 insertions(+), 137 deletions(-)
> 
> [ ... ]
> 
> So it's a series of 5 patches.  They LGTM.    And if  there's minor
> updates needed to address issues found once they're on the trunk, the
> consider those updates pre-approved.

Spooky, I pinged these patches the minute before you replied to this one.

> 
> Note that defining LOGICAL_OP_NON_SHORT_CIRCUIT and BRANCH_COST impact
> gimple code generation.  I'm a bit surprised there wasn't more fallout
> in the existing tests.

IIRC, when I tried messing with LOGICAL_OP_NON_SHORT_CIRCUIT and
BRANCH_COST before the full costs were implemented, they never had any
effect. I'm going to hand wave here and say that whatever behavior these
macros were affecting before the costs were implemented, was actually
behaving as it should. Then with the full costs implemented, I had to
tweak them to get back to that optimal state.

Also, the only conditional instructions available for MSP430 are
cbranch, i.e. there is never a choice between a cbranch and a cstore.
I was surprised BRANCH_COST had any effect, but after looking at output
assembly, it appeared that it did affect the number of cbranch insns
emitted, vs just other insns to move data about.

The changes did fix these optimization tests:
gcc.dg/tree-ssa/reassoc-33.c scan-tree-dump-times reassoc1 "Optimizing range tests" 3
gcc.dg/tree-ssa/reassoc-34.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1
gcc.dg/tree-ssa/reassoc-35.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1
gcc.dg/tree-ssa/reassoc-36.c scan-tree-dump-times reassoc1 "Optimizing range tests" 1

Thanks for the review, I'll watch for fallout after installing on trunk.
Jozef
> 
> jeff
> 
>