On 3/16/2026 8:13 AM, Yoshinori Sato wrote: > gcc/ChangeLog: > > * config/rx/predicates.md (rx_plt_call_operand): New predicate. > (rx_pcrel_label_operand): Likewisze. > * config/rx/rx.md (PIC_REG): New constant. > (UNSPEC_PIC): Likewise. > (UNSPEC_GOT): Likewise. > (UNSPEC_GOTOFF): Likewise. > (UNSPEC_PLT): Likewise. > (UNSPEC_GOTFUNCDESC): Likewise. > (UNSPEC_GOTOFFFUNCDESC): Likewise. > (UNSPEC_PCREL): Likewise. > (tablejump): Use relative in pic case. > (call): Add FDPIC call. > (call_value): Likewise. > (call_internal): Add use PICREG. > (call_value_internal): Likewise. > (mov<register_modes:mode>): Add PIC case. > (*mov<register_modes:mode>_internal): Added the condition that it is > not UNSPEC. > (movsi_pcrel): New. > (addsi3): Add PIC case. > (addsi_unspec): New. > (addsi3_flags): New. > (sym2GOT): New. > (sym2GOTreg): New. > (sym2GOTOFF): New. > (sym2GOTOFFreg): New. > (sym2GOTFUNCDESC): New. > (sym2GOTFUNCDESCreg): New. > (sym2GOTOFFFUNCDESC): New. > (sym2GOTOFFFUNCDESCreg): New. > (sym2PLT): New. > (call_fdpic): New. > (call_internal_fd): New. > (call_internal_plt): New. > (call_value_fdpic): New. > (call_value_internal_fd): New. > (call_value_internal_plt): New. > > Signed-off-by: Yoshinori Sato <[email protected]> > --- > gcc/config/rx/predicates.md | 12 ++ > gcc/config/rx/rx.md | 374 +++++++++++++++++++++++++++++++++--- > 2 files changed, 358 insertions(+), 28 deletions(-) > > diff --git a/gcc/config/rx/predicates.md b/gcc/config/rx/predicates.md > index 37fd63b538a..45248e3b7e5 100644 > --- a/gcc/config/rx/predicates.md > +++ b/gcc/config/rx/predicates.md > @@ -321,3 +321,15 @@ > (match_operand 0 "general_operand")) > (and (match_code "mem") > (match_operand 0 "rx_restricted_mem_operand")))) > + > +(define_predicate "rx_plt_call_operand" > + (match_code "const") > +) So this accepts any CONST. Is that really correct? I don't know enough about the port to be sure. > @@ -342,9 +351,9 @@ > (match_operand:SI 0 "register_operand" "r")) > (use (label_ref (match_operand 1 "" "")))] > "" > - { return TARGET_PID ? (TARGET_AS100_SYNTAX ? "\n?:\tbra\t%0" > - : "\n1:\tbra\t%0") > - : "\n1:jmp\t%0"; > + { return TARGET_PID || flag_pic ? (TARGET_AS100_SYNTAX ? "\n?:\tbra\t%0" > + : "\n1:\tbra\t%0") > + : "\n1:jmp\t%0"; This might need rewrapping. Our formatting standards use an 80 column max. > @@ -440,7 +458,8 @@ > (define_insn "call_internal" > [(call (mem:QI (match_operand:SI 0 "rx_call_operand" > "r,CALL_OP_SYMBOL_REF")) > (const_int 0)) > - (clobber (reg:CC CC_REG))] > + (clobber (reg:CC CC_REG)) > + (use (reg:SI PIC_REG))] So this means that CALL_INSNs have a reference to PIC_REG, even when not generating fdpic. Is that intentional? > > +(define_insn "movsi_pcrel" > + [(set (match_operand:SI 0 "register_operand" "=r") > + (match_operand:SI 1 "rx_pcrel_label_operand" "i")) > + (clobber (reg:CC CC_REG))] > + "flag_pic" > + "mvfc\tpc, %0 ! add\t%1 - . + 3, %0" > + [(set_attr "length" "6")] > +) Slightly concerned about this. Though with your efforts to move the rx port to use LRA, the requirement that this be handled by the movsi pattern wouldn't be a problem in the long term. > @@ -982,16 +1086,29 @@ > (set_attr "length" "2,2,2,3,4,5,6,2,3,3,4,5,6,5")] > ) > > -(define_insn "addsi3_pid" > +(define_insn "addsi3_unspec" > [(set (match_operand:SI 0 "register_operand" "=r") > - (plus:SI (match_operand:SI 1 "register_operand" "%0") > - (const:SI (unspec:SI [(match_operand:SI 2 > "immediate_operand" "i")] UNSPEC_PID_ADDR))))] > - "" > - "add\t%2, %0" > + (plus:SI (match_operand:SI 1 "register_operand" "r") > + (match_operand:SI 2 "immediate_operand" "i")))] > + "rx_is_unspec_offset(operands[2])" > + { > + return "add\t%2, %1, %0"; > + } > [(set_attr "length" "6") > (set_attr "timings" "11")] > ) > > +;; A helper to expand the above with the CC_MODE filled in. > +(define_expand "addsi3_flags" Are these going to need further adjustment as part of the LRA conversion? That's not a big problem, just an observation. > + > +(define_expand "symGOTFUNCDESC2reg" > + [(match_operand 0) (match_operand 1)] > + "TARGET_FDPIC" > + { > + rtx picreg = rx_get_fdpic_reg_initial_val(); > + rtx gotsym = gen_sym2GOTFUNCDESC (operands[1]); > + PUT_MODE (gotsym, Pmode); > + emit_move_insn (operands[0], > + gen_rtx_MEM(Pmode, gen_rtx_PLUS(Pmode, picreg, gotsym))); > + DONE; > + } That PUT_MODE is a red flag. Under what conditions is the got symbol not already in Pmode? In general if you find yourself using PUT_MODE, you're probably doing something wrong. Similarly for the other instances. > + > +(define_expand "call_value_fdpic" > + [(call (match_operand:QI 0 "general_operand") > + (match_operand:SI 1 "general_operand")) > + (use (match_operand 2 "" "")) > + (use (reg:SI PIC_REG))] Shouldn't this set a destination object? Or is it just mis-named? We use call_value to indicate the call as a return value. > + "" > + { > + rtx picreg = force_reg (Pmode, gen_rtx_REG(Pmode, PIC_REG)); So this is a general formatting nit that appears in many places. Always have a whitespace between the function name and the open paren for its arguments. I've only done a cursory review. You probably know the rx port as well as anyone currently working on GCC, so I'm willing to give a fair amount of freedom/leeway. We're definitely going to need a v2 and I'd tend to think landing the fdpic work after the LRA conversion would make more sense, particularly given the movsi situation. jeff
