Re: [RFC v2] RISCV: Combine Pass Clobber Ops

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Thu, Mar 10, 2022 at 09:54:55AM -0800, Patrick O'Neill wrote:
> I added this enforcement during the combine pass since it looks at the
> cost of certian expressions and can rely on the target to tell the
> pass that clobber-ops are cheaper than regular ops.

That is not a reason to put target stuff in generic code.  If you need
a new hook, you should create one.

> 	* combine.cc: Add register equality replacement.

Please Cc: the maintainers of any code you want to be looked at.

This is not suitable for stage 4.  Please try again in stage 1.

> 	* riscv.cc (riscv_insn_cost): Add in order to tell combine pass
> 	  that clobber-ops are cheaper.

(formatting)

> +/* Attempt to replace ops with clobber-ops.
> +     If the target implements clobber ops (set r1 (plus (r1)(r2))) as cheaper,

(formatting)

> +  if (!i0 && !i1 && i2 && i3 && GET_CODE(PATTERN(i2)) == SET

Space before opening paren (in essentially all cases).

> +      && GET_CODE(SET_DEST(PATTERN(i2))) == REG

REG_P

> +	// Now we have a dead operand register, and we know where the dest dies.

Don't mix comment styles.

> +	// Remove the note declaring the register as dead

Sentences end with a full stop.

> +	// Overwrite i2 dest with operand1
> +	rtx i2_dest = copy_rtx(operand1);
> +	SUBST (SET_DEST (PATTERN (i2)), i2_dest);

That comment only confuses matters?

> +	// Move the dest dead note to the new register
> +	note = find_reg_note (i3, REG_DEAD, prior_reg);
> +	if (note) {
> +	  remove_note (i3, note);
> +	  //add_reg_note (i3, REG_DEAD, op1_copy);
> +	}

Please don't submit unfinished code.  If there is any reason to comment
out code, it needs a comment itself.

> +static int
> +riscv_insn_cost (rtx_insn *insn, bool speed)
> +{
> +  rtx pat = PATTERN (insn);
> +
> +  if (TARGET_RVC && !speed) {

Opening curlies are on a new line, indented:
  if (a)
    {
      blablabla ();
      b0rk ();
    }

> +    if (GET_CODE(pat) == SET && GET_CODE(SET_DEST(pat)) == REG) {
> +      rtx src = SET_SRC(pat);
> +      rtx dest = SET_DEST(pat);
> +      if (GET_CODE(src) == PLUS && GET_CODE(XEXP(src, 0)) == REG && REGNO(XEXP(src, 0)) == REGNO(dest)) {

Line length is 80 chars maximum.

Comparing REGNOs isn't likely correct.  There can be pseudos here, but
also hard registers.  You need to consider both cases.  The code may
well be correct, but as written it isn't obvious at all.

> +	if (GET_CODE(XEXP(src, 1)) == REG)
> +	  return 2;
> +	else if (GET_CODE(XEXP(src, 1)) == CONST_INT && CMPRESD_OPERAND(INTVAL(XEXP(src, 1))))
> +	  return 2;

REG_P, CONST_INT_P

> +#define CMPRESD_OPERAND(VALUE) \
> +  (VALUE < 32 && VALUE >= -32)

IN_RANGE ((VALUE), -32, 31)

Using a predicate might be better?  Note you need parens around macro
params, btw.

> +/* True if VALUE is an unsigned 5-bit number. */
> +#define UNSIGNED_CMPRESD_OPERAND(VALUE) \
> +  (VALUE < 64 && VALUE >= 0)

IN_RANGE ((VALUE), 0, 63)


Segher