On Thu, Oct 1, 2015 at 8:08 AM, Segher Boessenkool
<seg...@kernel.crashing.org> wrote:
> After the shrink-wrapping patches the prologue will often be pushed
> "deeper" into the function, which in turn means the software trace cache
> pass will more often want to duplicate the basic block containing the
> prologue. This caused failures for 32-bit SVR4 with -msecure-plt PIC.
>
> This configuration uses the load_toc_v4_PIC_1 instruction, which creates
> assembler labels without using the normal machinery for that. If now
> the compiler decides to duplicate the insn, it will emit the same label
> twice. Boom.
>
> It isn't so easy to fix this to use labels the compiler knows about (let
> alone test that properly). Instead, this patch wires up a "cannot_copy"
> attribute to be used by TARGET_CANNOT_COPY_P, and sets that attribute on
> these insns we do not want copied.
>
> Bootstrapped and tested on powerpc64-linux, with the usual configurations
> (-m32,-m32/-mpowerpc64,-m64,-m64/-mlra); new testcase fails before, works
> after (on 32-bit).
>
> Is this okay for mainline?
Isn't that quite expensive? So even if not "easy", can you try?
Do we have other ports with local labels in define_insns? I see some in
darwin.md as well which your patch doesn't handle btw., otherwise
suspicious %0: also appears (only) in h8300.md. arc.md also has
a suspicious case in its doloop_end_i pattern.
Richard.
>
> Segher
>
>
> 2015-09-30 Segher Boessenkool <seg...@kernel.crashing.org>
>
> PR target/67788
> PR target/67789
> * config/rs6000/rs6000.c (TARGET_CANNOT_COPY_INSN_P): New.
> (rs6000_cannot_copy_insn_p): New function.
> * config/rs6000/rs6000.md (cannot_copy): New attribute.
> (load_toc_v4_PIC_1_normal): Set cannot_copy.
> (load_toc_v4_PIC_1_476): Ditto.
>
> gcc/testsuite/
> PR target/67788
> PR target/67789
> * gcc.target/powerpc/pr67789.c: New testcase.
>
> ---
> gcc/config/rs6000/rs6000.c | 11 +++++++++
> gcc/config/rs6000/rs6000.md | 9 +++++--
> gcc/testsuite/gcc.target/powerpc/pr67789.c | 39
> ++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr67789.c
>
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 93bb725..29fd198 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -1513,6 +1513,8 @@ static const struct attribute_spec
> rs6000_attribute_table[] =
> #define TARGET_REGISTER_MOVE_COST rs6000_register_move_cost
> #undef TARGET_MEMORY_MOVE_COST
> #define TARGET_MEMORY_MOVE_COST rs6000_memory_move_cost
> +#undef TARGET_CANNOT_COPY_INSN_P
> +#define TARGET_CANNOT_COPY_INSN_P rs6000_cannot_copy_insn_p
> #undef TARGET_RTX_COSTS
> #define TARGET_RTX_COSTS rs6000_rtx_costs
> #undef TARGET_ADDRESS_COST
> @@ -31226,6 +31228,15 @@ rs6000_xcoff_encode_section_info (tree decl, rtx
> rtl, int first)
> #endif /* HAVE_AS_TLS */
> #endif /* TARGET_XCOFF */
>
> +/* Return true if INSN should not be copied. */
> +
> +static bool
> +rs6000_cannot_copy_insn_p (rtx_insn *insn)
> +{
> + return recog_memoized (insn) >= 0
> + && get_attr_cannot_copy (insn);
> +}
> +
> /* Compute a (partial) cost for rtx X. Return true if the complete
> cost has been computed, and false if subexpressions should be
> scanned. In either case, *TOTAL contains the cost result. */
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index cfdb286..8c53c40 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -226,6 +226,9 @@ (define_attr "var_shift" "no,yes"
> (const_string "no"))
> (const_string "no")))
>
> +;; Is copying of this instruction disallowed?
> +(define_attr "cannot_copy" "no,yes" (const_string "no"))
> +
> ;; Define floating point instruction sub-types for use with Xfpu.md
> (define_attr "fp_type"
> "fp_default,fp_addsub_s,fp_addsub_d,fp_mul_s,fp_mul_d,fp_div_s,fp_div_d,fp_maddsub_s,fp_maddsub_d,fp_sqrt_s,fp_sqrt_d"
> (const_string "fp_default"))
>
> @@ -9130,7 +9133,8 @@ (define_insn "load_toc_v4_PIC_1_normal"
> && (flag_pic == 2 || (flag_pic && TARGET_SECURE_PLT))"
> "bcl 20,31,%0\\n%0:"
> [(set_attr "type" "branch")
> - (set_attr "length" "4")])
> + (set_attr "length" "4")
> + (set_attr "cannot_copy" "yes")])
>
> (define_insn "load_toc_v4_PIC_1_476"
> [(set (reg:SI LR_REGNO)
> @@ -9148,7 +9152,8 @@ (define_insn "load_toc_v4_PIC_1_476"
> return templ;
> }"
> [(set_attr "type" "branch")
> - (set_attr "length" "4")])
> + (set_attr "length" "4")
> + (set_attr "cannot_copy" "yes")])
>
> (define_expand "load_toc_v4_PIC_1b"
> [(parallel [(set (reg:SI LR_REGNO)
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr67789.c
> b/gcc/testsuite/gcc.target/powerpc/pr67789.c
> new file mode 100644
> index 0000000..d1bd047
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr67789.c
> @@ -0,0 +1,39 @@
> +/* { dg-do assemble } */
> +/* { dg-options "-O2 -msecure-plt -fPIC" } */
> +
> +#define FE_TONEAREST 0
> +#define FE_UPWARD 1
> +#define FE_DOWNWARD 2
> +#define FE_TOWARDZERO 3
> +
> +extern int fesetround(int);
> +
> +void
> +set_fpu_rounding_mode (int mode)
> +{
> + int rnd_mode;
> +
> + switch (mode)
> + {
> + case 2:
> + rnd_mode = FE_TONEAREST;
> + break;
> +
> + case 4:
> + rnd_mode = FE_UPWARD;
> + break;
> +
> + case 1:
> + rnd_mode = FE_DOWNWARD;
> + break;
> +
> + case 3:
> + rnd_mode = FE_TOWARDZERO;
> + break;
> +
> + default:
> + return;
> + }
> +
> + fesetround (rnd_mode);
> +}
> --
> 1.8.1.4
>
