Fix some typo inline, sorry for inconvenience. -----Original Message----- From: Li, Pan2 Sent: Monday, July 24, 2023 7:59 PM To: Robin Dapp <rdapp....@gmail.com>; gcc-patches@gcc.gnu.org Cc: juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, Yanzhang <yanzhang.w...@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....@gmail.com> Sent: Monday, July 24, 2023 6:29 PM To: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org Cc: rdapp....@gmail.com; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, Yanzhang <yanzhang.w...@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