| Message ID | 4D8A074E.8040009@gjlay.de |
|---|---|
| State | New |
| Headers | show |
> -----Original Message----- > From: Georg-Johann Lay [mailto:avr@gjlay.de] > Sent: Wednesday, March 23, 2011 8:45 AM > To: gcc-patches@gcc.gnu.org > Cc: Denis Chertykov; Anatoly Sokolov; Weddington, Eric > Subject: [Patch][AVR]: Use define_c_enum where appropriate > > Use define_c_enum instead of magic numbers to get unspec resp. > unspec_volatile constants. > Hi Johann, Can we hold off on this patch for just a little bit? I agree it would be good to clean it up, but I would like to get in the attached patch to add some builtin functions that Anatoly and I worked on. The patch was (nominally) for 4.4. I need to see if it will patch cleanly to trunk, and if not, then I'd like to keep the changes minimal. Eric
Weddington, Eric schrieb: > >> -----Original Message----- From: Georg-Johann Lay >> [mailto:avr@gjlay.de] Sent: Wednesday, March 23, 2011 8:45 AM To: >> gcc-patches@gcc.gnu.org Cc: Denis Chertykov; Anatoly Sokolov; >> Weddington, Eric Subject: [Patch][AVR]: Use define_c_enum where >> appropriate >> >> Use define_c_enum instead of magic numbers to get unspec resp. >> unspec_volatile constants. >> > > Hi Johann, > > Can we hold off on this patch for just a little bit? I agree it > would be good to clean it up, but I would like to get in the > attached patch to add some builtin functions that Anatoly and I > worked on. The patch was (nominally) for 4.4. I need to see if it > will patch cleanly to trunk, and if not, then I'd like to keep the > changes minimal. > > Eric Of course, it's nothing urgent. Just proceed with your work and commit this patchlet afterwards. Already thought about posting my updated built-in patch for avr backend. I'm digging in my patches from before my fsf assignment and found a bluit-in patch there... Johann
> -----Original Message----- > From: Georg-Johann Lay [mailto:avr@gjlay.de] > Sent: Wednesday, March 23, 2011 9:10 AM > To: Weddington, Eric > Cc: gcc-patches@gcc.gnu.org; Denis Chertykov; Anatoly Sokolov; Joerg > Wunsch > Subject: Re: [Patch][AVR]: Use define_c_enum where appropriate > > Of course, it's nothing urgent. Just proceed with your work and commit > this patchlet afterwards. > > Already thought about posting my updated built-in patch for avr > backend. I'm digging in my patches from before my fsf assignment and > found a bluit-in patch there... Then we should coordinate off-list with the other maintainers. I have a list of patches that I would like to get in as well. One of which I think was originally yours. :-) Eric
> +;; SWAP > +(define_insn "swap" > + [(set (match_operand:QI 0 "register_operand" "=r") > + (unspec:QI [(match_operand:QI 1 "register_operand" "0")] > + UNSPEC_SWAP))] > + "" > + "swap %0" > + [(set_attr "length" "1") > + (set_attr "cc" "none")]) This is already properly represented as (define_insn "*rotlqi3_4" [(set (match_operand:QI 0 "register_operand" "=r") (rotate:QI (match_operand:QI 1 "register_operand" "0") (const_int 4)))] "" "swap %0" [(set_attr "length" "1") (set_attr "cc" "none")]) you ought not need another copy; just have the builtin emit the rotate. > +(define_insn "fmuls" > + [(set (match_operand:HI 0 "a_register_operand" "=r") > + (unspec:HI [(match_operand:QI 1 "a_register_operand" "r") > + (match_operand:QI 2 "a_register_operand" "r")] > + UNSPEC_FMULS))] ... > +;; Registers from r16 to 24. > +(define_predicate "a_register_operand" > + (and (match_code "reg") > + (match_test "REGNO (op) >= 16 && REGNO (op) <= 24"))) These constraint/predicate pairs are wrong. You should use register_operand for all of the operands, and proper constraints. I believe that the output can simply use "=r", since HImode registers are already constrained to be aligned; the inputs should be "a". > + "AVR_HAVE_MUL" > + "fmuls %1,%2 > + movw %0,r0 > + clr r1" The rest of the port is fairly consistent using __zero_reg__ instead of r1. You probably want to continue that. r~
> -----Original Message----- > From: Richard Henderson [mailto:rth@redhat.com] > Sent: Wednesday, March 23, 2011 10:24 AM > To: Weddington, Eric > Cc: Georg-Johann Lay; gcc-patches@gcc.gnu.org; Denis Chertykov; Anatoly > Sokolov; Joerg Wunsch > Subject: Re: [Patch][AVR]: Use define_c_enum where appropriate > > > + "AVR_HAVE_MUL" > > + "fmuls %1,%2 > > + movw %0,r0 > > + clr r1" > > The rest of the port is fairly consistent using __zero_reg__ instead > of r1. You probably want to continue that. Yes, we will definitely need to use __zero_reg__ as there are new avr device variants (that need to be committed) where r0-r15 don't exist, which means that __zero_reg__ won't always be r1.
Weddington, Eric schrieb: > Hi Johann, > > Can we hold off on this patch for just a little bit? I agree it would > be good to clean it up, but I would like to get in the attached patch > to add some builtin functions that Anatoly and I worked on. The patch > was (nominally) for 4.4. I need to see if it will patch cleanly to > trunk, and if not, then I'd like to keep the changes minimal. Your patch is more elaborate, I just didn't know you planned to commit builtin support, nothing exciting in my work. + case AVR_BUILTIN_DELAY_CYCLES: + { + arg0 = CALL_EXPR_ARG (exp, 0); + op0 = expand_expr (arg0, NULL_RTX, VOIDmode, 0); + + if (!CONSTANT_P (op0)) + error ("__builtin_avr_delay_cycles expects an integer constant."); Should be if (!CONST_INT_P (op0)) error ("__builtin_avr_delay_cycles expects a compile time constant."); You use INTVAL in the insns, and symbols/addresses cannot be handled reasonably. AFAIR avr-libc casts float down to int, so no need to handle/cast float? + emit_insn (gen_delay_cycles (op0)); + return 0; + } + } + + for (i = 0, d = bdesc_1arg; i < ARRAY_SIZE (bdesc_1arg); i++, d++) + if (d->code == fcode) + return avr_expand_unop_builtin (d->icode, exp, target); + + for (i = 0, d = bdesc_1arg; i < ARRAY_SIZE (bdesc_2arg); i++, d++) -------------------------^ Tippo? + if (d->code == fcode) + return avr_expand_binop_builtin (d->icode, exp, target); Johann
Index: config/avr/avr.md =================================================================== --- config/avr/avr.md (Revision 171340) +++ config/avr/avr.md (Arbeitskopie) @@ -35,10 +35,6 @@ ;; ~ Output 'r' if not AVR_HAVE_JMP_CALL. ;; ! Output 'e' if AVR_HAVE_EIJMP_EICALL. -;; UNSPEC usage: -;; 0 Length of a string, see "strlenhi". -;; 1 Jump by register pair Z or by table addressed by Z, see "casesi". - (define_constants [(REG_X 26) (REG_Y 28) @@ -50,17 +46,22 @@ (define_constants (SREG_ADDR 0x5F) (RAMPZ_ADDR 0x5B) - - (UNSPEC_STRLEN 0) - (UNSPEC_INDEX_JMP 1) - (UNSPEC_SEI 2) - (UNSPEC_CLI 3) + ]) + +(define_c_enum "unspec" + [UNSPEC_STRLEN + UNSPEC_INDEX_JMP + UNSPEC_SEI + UNSPEC_CLI + ]) - (UNSPECV_PROLOGUE_SAVES 0) - (UNSPECV_EPILOGUE_RESTORES 1) - (UNSPECV_WRITE_SP_IRQ_ON 2) - (UNSPECV_WRITE_SP_IRQ_OFF 3) - (UNSPECV_GOTO_RECEIVER 4)]) +(define_c_enum "unspecv" + [UNSPECV_PROLOGUE_SAVES + UNSPECV_EPILOGUE_RESTORES + UNSPECV_WRITE_SP_IRQ_ON + UNSPECV_WRITE_SP_IRQ_OFF + UNSPECV_GOTO_RECEIVER + ]) (include "predicates.md") (include "constraints.md")