https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54089
--- Comment #85 from Oleg Endo <olegendo at gcc dot gnu.org> --- (In reply to Alexander Klepikov from comment #83) > Created attachment 55367 [details] > Collapsed libcall and additional loop move invariants patch v3 Thanks for staying on it! I've looked through the latest version of your patch. There are still formatting issues. I will not point out one by one at this time (but will so later in the end). For now, can you try to run the style check script on your changes? https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=contrib/check_GNU_style.sh and also briefly cross check with https://gcc.gnu.org/codingconventions.html As for the actual contents of the patch... > bool > expand_ashiftrt (rtx *operands) > { > int value = INTVAL (operands[2]) & 31; ^^^^^^ Missing check for 'const_int' operand. See other uses of 'CONST_INT_P'. > if (dump_file) > fprintf(dump_file, "ashrsi3: Emitting collapsed libcall\n"); This can be omitted. It will be obvious in the RTL dump after the expand pass because of the insn name, or not? 'expand_ashiftrt' is called only in the pattern 'define_expand "ashrsi3"', so we can move all the code into the expander, like it's done in e.g. 'define_expand "lshrsi3"'. Then the function 'expand_ashiftrt_individual' can be renamed back to 'expand_ashiftrt'. Actually, the 'define_expand "ashrsi3"' can just emit the 'ashrsi3_libcall_collapsed' directly. In other words, change the original 'expand_ashiftrt' function tail: > wrk = gen_reg_rtx (Pmode); > > /* Load the value into an arg reg and call a helper. */ > emit_move_insn (gen_rtx_REG (SImode, 4), operands[1]); > sprintf (func, "__ashiftrt_r4_%d", value); > rtx lab = function_symbol (wrk, func, SFUNC_STATIC).lab; > emit_insn (gen_ashrsi3_n (GEN_INT (value), wrk, lab)); > emit_move_insn (operands[0], gen_rtx_REG (SImode, 4)); > return true; to this: > emit_insn (gen_ashrsi3_libcall_collapsed (operands[0], operands[1], > GEN_INT(value))); > return true; ... but of course only if operand[2] is actually a 'const_int'. If operand[2] is not a constant, then the whole expander should fail and not emit anything. Which is what the original code was doing. In case of shifts by non-constant amounts, the middle-end will then expand the generic libcall for it (if I remember correctly). So basically, all we're doing here is adding a proxy pattern for the existing 'ashrsi3_n', that has a simpler structure and can be used by other passes like combine easier. Sounds plausible, but looking through the other shift patterns, they would all need that treatment? I think the 'define_insn "ashrsi3_libcall_collapsed"' and 'define_insn_and_split "ashrsi3_libcall_expand"' can be merged into a single pattern 'define_insn_and_split "ashrsi3_n_call" and the function 'expand_ashrsi3_libcall' (the tail of the original 'expand_ashiftrt') can be just put into the splitter code in the new "ashrsi3_n_call". The splitter condition should include "&& can_create_pseudo_p ()" to make sure it's ran before register allocation. I think this will reduce the change set a bit and make the intention of the patch a bit clearer. > +/* { dg-final { scan-assembler > "_f_loop1_rshift:.*mov\.l\\t(\.L\[0-9\]+),(r\[0-9\]+).*sts.l\\tpr,@-r15.*(\.L\[0-9\]+):.*jsr\\t@\\2.*bf\.s\\t\\3.*\\1:\\n\\t\.long\\t___ashiftrt_r4_6.*_f_loop2_rshift:" > { target { ! has_dyn_shift } } } } */ Can you try to somehow write this in a simpler way? Maybe omit some of the register number matches, as they don't matter etc.
