RE: [PATCH v6] RISC-V: Support CALL for RVV floating-point dynamic rounding

["Li, Pan2" <pan2.li@intel.com>]

Fix some typo inline,  sorry for inconvenience.

-----Original Message-----
From: Li, Pan2 
Sent: Monday, July 24, 2023 7:59 PM
To: Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: RE: [PATCH v6] RISC-V: Support CALL for RVV floating-point dynamic rounding

Thanks Robin for comments.

>> Does prev_nonnote_nondebug_insn_bb help here?  In general, we scan
>> back here to the last insn and "recover" if it was a call?  Why
>> can't we set the proper value already before exiting the function?

Good to learn prev_nonnote_nondebug_insn_bb, will have a try here.
First we mark the call as DYN_CALL. We expect the insn next to the call to emit the backup insn.
However, if next instruction is DYN_NONE, the mode switching it self will never call emit. Thus, for each
Insn is DYN_NONE, we will reset it to DYN if the last insn is call and let the mode switching to emit properly.

When meet a call, we assume the rounding mode will be clobbered.

>> What about nondebug insns?  Also maybe next_nonnote_nondebug_insn_bb
>> helps?  How about splitting the function in detection and emit?
>> I.e. bb_ends_in_call (or so) and the emit part.  That way it
>> could be more obvious in "needed" what needs to be done.

The point here is that the call may be the last insn of current bb. Then we have chance to emit backup insn AFTER
the call, because the emit by default emit the insn before current insn.

Thus we try to make up this during the needed process, I know this may not be a good idea but it is the only way
I can locate shortly. I may try to find another way if we can control the insn emit location for each insn.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Monday, July 24, 2023 6:29 PM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: [PATCH v6] RISC-V: Support CALL for RVV floating-point dynamic rounding

Hi Pan,

> +  for (insn = PREV_INSN (cur_insn); insn; insn = PREV_INSN (insn))
> +    {
> +      if (INSN_P (insn))
> +	{
> +	  if (CALL_P (insn))
> +	    mode = FRM_MODE_DYN;
> +	  break;
> +	}
> +
> +      if (insn == BB_HEAD (bb))
> +	break;
> +    }
> +
> +  return mode;
> +}

Does prev_nonnote_nondebug_insn_bb help here?  In general, we scan
back here to the last insn and "recover" if it was a call?  Why
can't we set the proper value already before exiting the function?

I guess the more general question is more towards call-clobbered or
not?  In this patch we assume the rounding mode is call clobbered
and restore it ourselves.  Has there been any kind of consensus
on this?  Intuitively I would have expected a function that requires
a non-standard rounding mode to set and restore it itself. 

> +
> +/* Insert the backup frm insn to the end of the bb if and only if the call
> +   is the last insn of this bb.  */
> +
> +static void
> +riscv_frm_reconcile_call_as_bb_end (rtx_insn *cur_insn)
> +{
> +  rtx_insn *insn;
> +  basic_block bb = BLOCK_FOR_INSN (cur_insn);
> +
> +  gcc_assert (CALL_P (cur_insn));
> +
> +  if (cur_insn != BB_END (bb))
> +    {
> +      for (insn = NEXT_INSN (cur_insn); insn; insn = NEXT_INSN (insn))
> +	{
> +	  if (INSN_P (insn)) /* If there is one insn after call, do nothing.  */
> +	    return;

What about nondebug insns?  Also maybe next_nonnote_nondebug_insn_bb
helps?  How about splitting the function in detection and emit?
I.e. bb_ends_in_call (or so) and the emit part.  That way it
could be more obvious in "needed" what needs to be done.

Are we handling sibcalls and tail calls properly here?

In general I'm still a bit wary that we are checking mode != prev_mode
but cannot really pinpoint why.  I would have hoped that the generic
code only calls us when this check is unnecessary and if not the
"needed" hook should be adjusted.

I also find it a bit odd to emit instructions when checking
if another mode is needed.  If what's required cannot be accomplished
with the current common code, shouldn't we rather try to amend that?

Regards
 Robin