New port^2: Renesas RL78

[DJ Delorie]

> > (define_expand "zero_extendqihi2"
> >   [(set (match_operand:HI                 0 "nonimmediate_operand")
> >         (zero_extend:HI (match_operand:QI 1 "general_operand")))]
> >   ""
> >   "if (rl78_force_nonfar_2 (operands, gen_zero_extendqihi2))
> >      DONE;"
> >   )
> 
> You should be able to simply rl78_nonfar_operand etc and skip
> the indirect expansion business.  I don't know when you started
> the port but Richard Sandiford's expansion cleanup dated 2011-03-23
> should have fixed whatever you're working around in this file.

The problem I'm trying to solve with that is that there's only one
segment register (ES) so you only need to force an operand non-far if
*both* operands are far.  Not sure if the function is implemented that
way, but I coded the expanders that way.

> >     if (CONST_INT_P (operand1) && ! IN_RANGE (INTVAL (operand1), (-1 << 8) + 1, (1 << 8) - 1))
> >       FAIL;
> 
> Huh?  This would be an assert, not a FAIL.  But why have it?
> 
> It sounds like something that should have been caught way up
> the chain...

"should" he says ;-)

> > (define_expand "umulqihi3"
> >   [(set (match_operand:HI 0 "register_operand")
> >         (mult:HI (zero_extend:HI (match_operand:QI 1 "register_operand"))
> >                  (zero_extend:HI (match_operand:QI 2 "register_operand"))))]
> >   "0"
> >   ""
> > )
> 
> Just delete it?

Perhaps.  Old code, I'd have to compare with the two hardware
multipliers (and/or contemplate a libgcc implementation) to see.

> > (define_expand "addsi3"
> >   [(set (match_operand:SI          0 "register_operand" "=&v")
> >         (plus:SI (match_operand:SI 1 "nonmemory_operand" "vi")
> >                  (match_operand    2 "nonmemory_operand" "vi")))
> >    ]
> >   ""
> >   "if (!nonmemory_operand (operands[1], SImode))
> >      operands[1] = force_reg (SImode, operands[1]);
> >    if (!nonmemory_operand (operands[1], SImode))
> >      operands[2] = force_reg (SImode, operands[2]);"
> > )
> 
> Drop the register constrains in the expander.  Use register_operand
> in the expander and drop the force_reg bits.

The pattern *does* accept immediates as-is.  Not sure why that extra
check is in there, though...  might be that parts of gcc call
gen_addsi3() without checking the predicates first.  I've seen it do
that for moves.

> I'm somewhat surprised you don't implement subsi3 as well...

There's probably a lot of SImode ops I haven't implemented yet.
addsi3 was a huge chunk of time in coremark, so it got attention
sooner.

> It sure looks like 99% of the *_real and *_virt insn patterns should
> have "*" names so that they don't generate unused expanders.

Yup.

> >   if (cfun->machine->trampolines_used)
> >     emit_insn (gen_trampoline_uninit ());
> 
> Do you have another way to uninit, based on stack level?  Otherwise
> this feature is incompatible with longjmp or nonlocal goto.  Not
> that it isn't a fairly decent plan in the circumstances...

Yes, it's just like alloca() - it detects a stack shrink in the next
call to the library function and frees up any stubs that are "off the
stack".  The call in the epilogue is to increase the odds of properly
detecting such a case, since we know at that point it's out of scope.

> > #define FAILED
> > #define MAYBE_OK(insn) if (insn_ok_now (insn)) return;
> 
> That non-fallback FAILED probably still ought to be gcc_unreachable.

No, that's a status indicator for debugging that it hasn't matched
*yet*.  Probably should get renamed to be more obvious what it's for
though ;-)

> I didn't review the devirt pass in detail.  I'm somewhat surprised
> that you still get decent debug info post-devirt, since I would think
> you'd need to copy more of the REG_ATTRs data into the new hard regs.

I suspect I do, but I don't know how to do that yet.

> The port as a whole looks pretty good though.

Thanks!