On 5/29/19 7:40 AM, Segher Boessenkool wrote:
> 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!
Thanks! Will make appropriate changes and commit. Much obliged for the
review!
Bill
>
>
> Segher
>
