This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Add support for R_MIPS_JALR
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Adam Nemet <anemet at caviumnetworks dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Tue, 15 Sep 2009 20:16:11 +0100
- Subject: Re: [PATCH] Add support for R_MIPS_JALR
- References: <19118.39785.790622.796631@ropi.home>
Excellent patch, thanks.
Adam Nemet <anemet@caviumnetworks.com> writes:
> * After DF determines the call symbol I need to save it somewhere until
> asm-output time. After considering many things (MEM_EXPR, REG_EXPR,
> CALL_INSN_FUNCTION_USAGE, explicit use in the parallel) I decided to extend
> the opaque args-size operand of the call expression into an optional
> call-attributes field and stick the symbol there. So now the second op of a
> call-expr can hold an UNSPEC_CALL_ATTR or args-size as before. As you can
> see, this allows very little change to mips.md and MIPS_CALL.
Nice! I agree this should be the best approach from a correctness
viewpoint too, since the annotation becomes (an opaque) part of the
new calls' semantics. I can't see any legitimate way in which the
information could be lost.
> * I had to tweak CFG recreation in md_reorg. We need this before splitting
> insns if the DF will be used. See the comment in mips_reorg.
Yeah, I like the way you've split out the df stuff into a separate routine.
> +mrelax-pic-calls
> +Target Report Mask(RELAX_PIC_CALLS)
> +Try to turn PIC (indirect) calls into direct calls
"Try to allow the linker to turn PIC calls into branches"? Bit clumsy,
I know, but the current version seems to suggest that GCC can do the
change itself, rather than simply emitting a hint for the linker.
Better suggestions welcome!
> + x = PATTERN (insn);
> + if (GET_CODE (x) == PARALLEL)
> + x = XVECEXP (x, 0, 0);
> + if (GET_CODE (x) == SET)
> + x = XEXP (x, 1);
^^^^^
formatting
> + symbol = mips_pic_call_symbol_from_set (defs->ref, reg);
> + if (!symbol)
> + return NULL_RTX;
> +
> + /* If we have more than one definition, they need to be identical. */
> + for (defs = defs->next; defs; defs = defs->next)
> + {
> + rtx other;
> +
> + other = mips_pic_call_symbol_from_set (defs->ref, reg);
> + if (!rtx_equal_p (symbol, other))
> + return NULL_RTX;
> + }
I'd be tempted to split this out into a new function and use it for
the "reg-to-reg" copy case in mips_pic_call_symbol_from_set too.
Things like scheduling could in principle introduce deliberate
redundancies here, and since it's such an easy change to make...
> + /* Only optimize PIC indirect calls if they are actually required. */
> + if (!TARGET_USE_PIC_FN_ADDR_REG)
> + target_flags &= ~MASK_RELAX_PIC_CALLS;
This is fine as far as correctness goes, but how about
"!TARGET_USE_GOT || !TARGET_EXPLICIT_RELOCS" instead?
That's the condition used in MIPS_CALL, and should avoid
some unnecessary work.
> +Try to turn PIC calls that are normally dispatched via register
> +@code{$25} into direct calls. This is only possible if the linker can
> +resolve the destination at link-time and if the desination is within
^^^^^^^^^^
missing 't'
> + mips_option_dependency options "-mrelax-pic-calls" "-mexplicit-relocs"
> + mips_option_dependency options "-mrelax-pic-calls" "-mno-plt"
*Very* minor, but could you swap these two around? I'd like to keep
entries with the same lhs sorted by rhs.
Glad to see it fits so neatly into the flow though. ;)
Why not imply -mabicalls too, rather than list it in every test?
> +/* { dg-final { scan-assembler "\.reloc\t1f,R_MIPS_JALR,normal\n1:\tjalr\t" } } */
> +/* { dg-final { scan-assembler "\.reloc\t1f,R_MIPS_JALR,normal2\n1:\tjalr\t" } } */
> +/* { dg-final { scan-assembler "\.reloc\t1f,R_MIPS_JALR,staticfunc\n1:\tjalr\t" } } */
> +/* { dg-final { scan-assembler "\.reloc\t1f,R_MIPS_JALR,tail\n1:\tjr\t" } } */
> +/* { dg-final { scan-assembler "\.reloc\t1f,R_MIPS_JALR,tail2\n1:\tjr\t" } } */
Hmm, I see there are other instances of this in gcc.target/mips
(as well as some correct instances), but... "\\." rather than "\.".
Single "\"s are normal C escapes; you need "\\" for a regexp escape.
As far as the generic testsuite changes go, I've a slight preference for
adding -mno-relax-pic-calls instead skipping tests, but I don't feel too
strongly about it.
The most important thing though is that we can't do this for MIPS16.
MIPS16 register jumps are 2 bytes long, and it doesn't have a BAL
instruction. So mips_set_mips16_mode should unconditionally
(and silently) clear MASK_RELAX_PIC_CALLS. I suppose we should
also then drop:
> + /* Don't check for PIC_FUNCTION_ADDR_REGNUM here. MIPS16 calls through
> + one of the MIPS16 regs. */
although I think the code itself should stay as it is, since it's
both simple and (AFAICT) correct.
OK with those changes, thanks.
Richard