Re: [PATCH 09/44] RISC-V: Rework branch costing model for if-conversion

["Maciej W. Rozycki" <macro@embecosm.com>]

On Tue, 28 Nov 2023, Jeff Law wrote:

> FWIW, I was looking at a regression with our internal tests after your
> changes.   It was quite nice to see how well twiddling -mbranch-cost
> correlated to how many instructions we would allow in a conditional move
> sequence.

 I'm a bit concerned though that our interpretation of `-mbranch-cost=0' 
is different from the middle end's, such as in `emit_store_flag':

  /* If we reached here, we can't do this with a scc insn, however there
     are some comparisons that can be done in other ways.  Don't do any
     of these cases if branches are very cheap.  */
  if (BRANCH_COST (optimize_insn_for_speed_p (), false) == 0)
    return 0;

> The downside is it highlighted the gimple vs RTL use issue.  I'm confident
> that we would like to see a higher branch cost in the RTL phases for our
> uarch, but I'm much less comfortable with how that's going to change the
> decisions made in trees/gimple.  We'll have to investigate that at some depth.

 Ack.

> >  I've looked at it already and it's the middle end that ends up with the
> > zero-extension, specifically `convert_move' invoked from `emit_cstore'
> > down the call to `noce_try_store_flag_mask', to widen the output from
> > `cstoredi4', so I don't think we can do anything in the backend to prevent
> > it from happening.  And neither I think we can do anything useful about
> > `cstoredi4' having a SImode output, as it's a pattern matched by name
> > rather than RTX, so we can't provide variants having a SImode and a DImode
> > output each both at a time, as that would cause a name clash.
> We're actually tracking some of these extraneous extensions.  Do you happen to
> know if the zero-extended object happens to be (subreg:SI (reg:DI)) kind of
> construct?  That's the kind of thing we're chasing down right now from various
> points.  Vineet has already fixed one class of them.  Jivan and I are looking
> at others.

 Under GDB it's a plain move from (reg:SI 140) to (reg:DI 139), as in the 
FROM and TO arguments to `convert_move' respectively.  This makes it call 
`convert_mode_scalar', which then chooses between `zext_optab' and 
`sext_optab' as appropriate, under:

  /* If the target has a converter from FROM_MODE to TO_MODE, use it.  */

to produce:

(set (reg:DI 139)
    (zero_extend:DI (reg:SI 140)))

ending up with this complete sequence:

(insn 27 0 28 (set (reg:SI 140)
        (eq:SI (reg/v:DI 137 [ c ])
            (const_int 0 [0]))) -1
     (nil))
(insn 28 27 29 (set (reg:DI 139)
        (zero_extend:DI (reg:SI 140))) -1
     (nil))
(insn 29 28 30 (set (reg:DI 141)
        (neg:DI (reg:DI 139))) -1
     (nil))
(insn 30 29 0 (set (reg/v:DI 134 [ <retval> ])
        (and:DI (reg/v:DI 135 [ a ])
            (reg:DI 141))) -1
     (nil))

passed to `targetm.noce_conversion_profitable_p' right away.  Maybe you 
can teach `emit_cstore' or `convert_move' to use a subreg when it is known
for the particular target that the value produced by the conditional-set 
machine instruction emitted by `cstoreMODE4' is valid unchanged in both 
modes.

 You can fiddle with it by trying:

$ gcc -march=rv64gc -mbranch-cost=3 -O2 -S gcc/testsuite/gcc.target/riscv/pr105314.c

Set a breakpoint at `noce_try_store_flag_mask' and then single-step to see 
how things proceed.

  Maciej