On неделя, 9 юни 2019 г. 17:38:58 EEST Segher Boessenkool wrote: > Hi Dimitar, > > Just some comments, do with it what you want :-) > > On Sun, Jun 09, 2019 at 11:01:38PM +0300, Dimitar Dimitrov wrote: > > +; An unfortunate side effect is that quite a few invalid RTL patterns are > > +; generated. For example: > > +; ... (zero_extend:SI (match_operand:SI ...)) ... > > You could perhaps use a mode iterator like rs6000's EXTHI and friends. The machine description already extensively uses mode iterators.
The example in that comment is written with SI mode for brevity. > > > +; These patterns are harmless since no pass should generate such RTL. > > This +; shortcut allows us to keep small and concise machine description > > patterns. > But the generated code (of GCC itself) will be larger. It's also not > harmless in that it can complicate your debugging, problems take longer > to spot. I'm afraid that's the best I can do without tooling upgrades. Reference: https://gcc.gnu.org/ml/gcc-patches/2018-09/msg00688.html > > > +(define_insn > > "add_impl<EQD:mode><EQS0:mode><EQS1:mode>_<alu3_zext><alu3_zext_op1><alu3 > > _zext_op2>" > Many lines are too long. Not that it is clear how to fix that in this > particular case ;-) Those ones I'm not sure how to break. The C code should conform to GNU standards, though. > > > + [(set_attr "type" "alu,alu,alu")]) > > You can just say > > [(set_attr "type" "alu")]) > > if all alternatives are the same value for an attribute. I'll fix it. Thanks. > > > +(define_insn "one_impl<EQD:mode><EQS0:mode>_<alu2_zext>" > > The standard pattern is called one_cmpl, so maybe you want one_cmpl_impl > here? one_impl looks like a typo. I'll update to one_cmpl_impl. Thanks. > > > +(define_subst "alu2_zext_subst" > > + [(set (match_operand:EQD 0 "" "") > > + (ALUOP2:EQD (zero_extend:EQD (match_operand:EQD 1 "" ""))))] > > I don't know if this actually works for define_subst, but at least in many > other cases you can write this like > > (define_subst "alu2_zext_subst" > [(set (match_operand:EQD 0) > (ALUOP2:EQD (zero_extend:EQD (match_operand:EQD 1))))] > > (you can omit trailing empty string arguments). Indeed, genmddump generates the same output with or without the empty strings. I was not sure which is the preferred form, though. I see lots of MD definitions in the GCC tree with empty strings. > > > +(define_predicate "pru_muldst_operand" > > + (match_code "subreg,reg") > > +{ > > + if (register_operand (op, mode)) > > + { > > + int regno; > > + > > + if (REG_P (op)) > > + regno = REGNO (op); > > + else if (GET_CODE (op) == SUBREG && REG_P (SUBREG_REG (op))) > > + regno = REGNO (SUBREG_REG (op)); > > + else > > + return 0; > > + > > + return REGNO_REG_CLASS (regno) == MULDST_REGS > > + || regno >= FIRST_PSEUDO_REGISTER; > > + } > > + return 0; > > +}) > > > > > > +static bool > > +pru_hard_regno_scratch_ok (unsigned int regno) > > +{ > > + /* Don't allow hard registers that might be part of the frame pointer. > > + Some places in the compiler just test for > > [HARD_]FRAME_POINTER_REGNUM > > + and don't handle a frame pointer that spans more than one register. > > + TODO: Fix those faulty places. */ > > + > > + if ((!reload_completed || frame_pointer_needed) > > + && ((regno >= HARD_FRAME_POINTER_REGNUM > > + && regno <= HARD_FRAME_POINTER_REGNUM + 3) > > + || (regno >= FRAME_POINTER_REGNUM > > + && regno <= FRAME_POINTER_REGNUM + 3))) > > + return false; > > Use IN_RANGE? Sounds good. Will rewrite it. > > > + /* QBxx conditional branching cannot cope with block reordering. */ > > + if (flag_reorder_blocks_and_partition) > > + { > > + inform (input_location, "%<-freorder-blocks-and-partition%> " > > + "not supported on this architecture"); > > + flag_reorder_blocks_and_partition = 0; > > + flag_reorder_blocks = 1; > > + } > > What you cannot cope with is the hot/cold partitioning, I guess -- > otherwise you'd have to disable reorder_blocks itself, and that would > result in pretty terrible code. Yes. > > > +; There is no pipeline, so our scheduling description is simple. > > +(define_automaton "pru") > > +(define_cpu_unit "cpu" "pru") > > + > > +(define_insn_reservation "everything" 1 (match_test "true") "cpu") > > Because you have a scheduling description, INSN_SCHEDULING is defined, > and that makes combine not create SUBREGs of MEM. Which is pretty > important :-) Would lack of INSN_SCHEDULING result in a more efficient target code? Is it recommended? I added dummy scheduling as a precaution to avoid issues like PR78883 for AVR. BTW, today I tested with and without scheduling description for PRU. I didn't get any new testsuite failures. > > > It looks like a quite complete port, congratulations :-) > > > Segher Thank you, Dimitar
