Hi Bill,
On Thu, May 23, 2019 at 09:11:44PM -0500, Bill Schmidt wrote:
> (1) When a function uses PC-relative code generation, all direct calls (other
> than
> sibcalls) that the function makes to local or external callees should appear
> as
> "bl sym@notoc" and should not be followed by a nop instruction. @notoc
> indicates
> that the assembler should annotate the call with R_PPC64_REL24_NOTOC, meaning
> that the caller does not guarantee that r2 contains a valid TOC pointer. Thus
> the linker should not try to replace a subsequent "nop" with a TOC restore
> instruction.
All necessary linker (and binutils and GAS) support is upstream already, right?
> In creating the new sibcall patterns, I did not duplicate the "c" alternatives
> that allow for bctr or blr sibcalls. I don't think there's a way to generate
> those currently. The bctr would be legitimate for PC-relative sibcalls if you
> can prove that the target function is in the same binary, but we don't appear
> to detect that possibility today.
But you could see that the target is in the same translation unit, for example?
That should be a simple test to make, too.
> pld 12,0(0),1
> .reloc .-8,R_PPC64_PLT_PCREL34_NOTOC,foo
Are we guaranteed the assembler always writes a pld like this as 8 bytes?
> * gcc.target/powerpc/notoc-direct-1.c: New.
> * gcc.target/powerpc/pcrel-sibcall-1.c: New.
A few more testcases would be useful. Well we'll gain a lot of-em soon
enough, I suppose.
> static char str[32]; /* 2 spare */
> - if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
> + if (rs6000_pcrel_p (cfun))
> + sprintf (str, "b%s %s@notoc%s", sibcall ? "" : "l", z, arg);
> + else if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
> sprintf (str, "b%s %s%s%s", sibcall ? "" : "l", z, arg,
> sibcall ? "" : "\n\tnop");
Two spare, and you add one char (@notoc vs. ..nop), so at a minimum you
need to correct the comment?
> + if (DEFAULT_ABI == ABI_V4
> + && (!TARGET_SECURE_PLT
> + || !flag_pic
> + || (decl
> + && (*targetm.binds_local_p) (decl))))
> + return true;
> +
> + return false;
Please invert this (put the "return false" ondition in the if, like the
preceding comment says).
> if (TARGET_PLTSEQ)
> {
> rtx base = const0_rtx;
> - int regno;
> - if (DEFAULT_ABI == ABI_ELFv2)
> + int regno = 12;
> + if (rs6000_pcrel_p (cfun))
> {
> - base = gen_rtx_REG (Pmode, TOC_REGISTER);
> - regno = 12;
> + rtx reg = gen_rtx_REG (Pmode, regno);
> + rtx u = gen_rtx_UNSPEC (Pmode, gen_rtvec (3, base, call_ref, arg),
> + UNSPEC_PLT_PCREL);
> + emit_insn (gen_rtx_SET (reg, u));
> + return reg;
> }
You don't need a regno variable here, so don't use it, only set it later
where it _is_ used?
> +(define_insn "*pltseq_plt_pcrel<mode>"
> + [(set (match_operand:P 0 "gpc_reg_operand" "=r")
> + (unspec:P [(match_operand:P 1 "" "")
> + (match_operand:P 2 "symbol_ref_operand" "s")
> + (match_operand:P 3 "" "")]
> + UNSPEC_PLT_PCREL))]
> + "HAVE_AS_PLTSEQ && TARGET_TLS_MARKERS
> + && rs6000_pcrel_p (cfun)"
> +{
> + return rs6000_pltseq_template (operands, 4);
Maybe those "4" magic constants should be an enum?
> +int zz0 ()
> +{
> + asm ("");
> + return 16;
> +};
You might want to put in a comment what this asm is for.
Please consider those things. Okay for trunk with that. Thanks!
Segher
