On 21 October 2023 01:56:16 CEST, Vineet Gupta <vine...@rivosinc.com> wrote:
On 10/19/23 23:50, Ajit Agarwal wrote:
Hello All:
This version 9 of the patch uses abi interfaces to remove zero and sign
extension elimination.
Bootstrapped and regtested on powerpc-linux-gnu.
In this version (version 9) of the patch following review comments are
incorporated.
a) Removal of hard code zero_extend and sign_extend in abi interfaces.
b) Source and destination with different registers are considered.
c) Further enhancements.
d) Added sign extension elimination using abi interfaces.
As has been trend in the past, I don't think all the review comments have been
addressed.
And apart from that, may I ask if this is just me, or does anybody else think
that it might be worthwhile to actually read a patch before (re-)posting?
Seeing e.g. the proposed abi_extension_candidate_p as written in a first POC
would deserve some manual CSE, if nothing more then for clarity and conciseness?
Just curious from a meta perspective..
And:
ree: Improve ree pass for rs6000 target using defined abi interfaces
mentioning powerpc like this, and then changing generic code could be
interpreted as misleading, IMHO.
For rs6000 target we see redundant zero and sign extension and done
to improve ree pass to eliminate such redundant zero and sign extension
using defined ABI interfaces.
Mentioning powerpc in the body as one of the affected target(s) is of course
fine.
+/* Return TRUE if target mode is equal to source mode of zero_extend
+ or sign_extend otherwise false. */
, false otherwise.
But I'm not a native speaker
+/* Return TRUE if the candidate insn is zero extend and regno is
+ a return registers. */
+
+static bool
+abi_extension_candidate_return_reg_p (/*rtx_insn *insn, */int regno)
Leftover debug comment.
+{
+ if (targetm.calls.function_value_regno_p (regno))
+ return true;
+
+ return false;
+}
+
As said, I don't see why the below was not cleaned up before the V1 submission.
Iff it breaks when manually CSEing, I'm curious why?
+/* Return TRUE if reg source operand of zero_extend is argument registers
+ and not return registers and source and destination operand are same
+ and mode of source and destination operand are not same. */
+
+static bool
+abi_extension_candidate_p (rtx_insn *insn)
+{
+ rtx set = single_set (insn);
+ machine_mode dst_mode = GET_MODE (SET_DEST (set));
+ rtx orig_src = XEXP (SET_SRC (set), 0);
+
+ if (!FUNCTION_ARG_REGNO_P (REGNO (orig_src))
+ || abi_extension_candidate_return_reg_p (/*insn,*/ REGNO (orig_src)))
On top, debug leftover.
+ return false;
+
+ /* Mode of destination and source should be different. */
+ if (dst_mode == GET_MODE (orig_src))
+ return false;
+
+ machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0));
+ bool promote_p = abi_target_promote_function_mode (mode);
+
+ /* REGNO of source and destination should be same if not
+ promoted. */
+ if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src))
+ return false;
+
+ return true;
+}
+
As said, please also rephrase the above (and everything else if it obviously
looks akin the above).
The rest, mentioned below, should largely be covered by following the coding
convention.
+/* Return TRUE if the candidate insn is zero extend and regno is
+ an argument registers. */
Singular register.
+
+static bool
+abi_extension_candidate_argno_p (/*rtx_code code, */int regno)
Debug leftover.
I would probably have inlined this function manually, with a respective comment.
Did not look how often it is used, admittedly.
+{
+ if (FUNCTION_ARG_REGNO_P (regno))
+ return true;
+
+ return false;
+}
[]
+
/* This function goes through all reaching defs of the source
s/This function goes/Go/
of the candidate for elimination (CAND) and tries to combine
(of, ?didn't look) candidate CAND for eliminating
the extension with the definition instruction. The changes
defining
Pre-existing, I know.
But you could fix those in a preparatory patch while you touch surrounding code.
This is not a requirement, of course, just good habit, IMHO.
@@ -770,6 +889,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat,
ext_state *state)
state->defs_list.truncate (0);
state->copies_list.truncate (0);
+ rtx orig_src = XEXP (SET_SRC (cand->expr),0);
+
+ if (abi_extension_candidate_p (cand->insn)
+ && (!get_defs (cand->insn, orig_src, NULL)))
Excess braces.
Hopefully check_gnu_style would have complained.
+ return abi_handle_regs (cand->insn);
outcome = make_defs_and_copies_lists (cand->insn, set_pat, state);
@@ -1036,6 +1160,15 @@ combine_reaching_defs (ext_cand *cand, const_rtx
set_pat, ext_state *state)
}
}
+ rtx insn_set = single_set (cand->insn);
+
+ machine_mode mode = (GET_MODE (XEXP (SET_SRC (insn_set), 0)));
Excess braces.
Also in a lot of other spots in your patch.
Please read the coding conventions and the patch, once again, before submission?
thanks
