This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Add support for R_MIPS_JALR


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]