https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68277
Oleg Endo <olegendo at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|UNCONFIRMED |NEW
Last reconfirmed| |2015-11-11
Ever confirmed|0 |1
--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Kazumoto Kojima from comment #2)
Thanks for looking at this Kaz.
> Here is a workaround. It changes the total text size of CSiBE
> from 3310543 to 3310579 which looks simply noise.
Most of the cases go like this:
before:
mov r0,r1 <<< initial load of r1
add #-65,r1
cmp/hi r10,r1
bt/s .L345
add #-22,r1 <<< r1 reused
after:
mov r0,r1 <<< inital load of r1
add #-65,r1
cmp/hi r10,r1
mov r0,r1 <<< r1 reloaded again
bt/s .L345
add #-87,r1 <<< bigger constant
But as you said, there are only a few cases throughout CSiBE, so it doesn't
seem worth the trouble.
>
> diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
> index 557a0f0..ad39a3c 100644
> --- a/gcc/config/sh/sh.md
> +++ b/gcc/config/sh/sh.md
> @@ -2236,6 +2236,23 @@
> }
> [(set_attr "type" "arith")])
>
> +;; Old reload might generate add insns directly for the memory address
> +;; of complex insns like synchronization insns when reloading.
> +(define_insn_and_split "*addsi3"
> + [(set (match_operand:SI 0 "arith_reg_dest" "=r")
> + (plus:SI (match_operand:SI 1 "arith_reg_operand" "r")
> + (match_operand:SI 2 "const_int_operand" "I08")))]
> + "TARGET_SH1 && !sh_lra_p ()
> + && reload_completed
> + && !reg_overlap_mentioned_p (operands[0], operands[1])"
> + "#"
> + "&& reload_completed"
> + [(set (match_dup 0) (match_dup 1))
> + (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))]
> +{
> +}
> + [(set_attr "type" "arith")])
> +
> (define_insn_and_split "*addsi3"
> [(set (match_operand:SI 0 "arith_reg_dest" "=r,r")
> (plus:SI (match_operand:SI 1 "arith_reg_operand" "%0,r")
>
> though I'm not sure that it's ok to add that hack to fix this corner
> case.
I don't have any alternative ideas either. However, I think it's better to
also allow reg = reg + reg, instead of only allowing reg = reg + imm8:
Index: gcc/config/sh/sh.md
===================================================================
--- gcc/config/sh/sh.md (revision 230158)
+++ gcc/config/sh/sh.md (working copy)
@@ -2236,7 +2236,22 @@
}
[(set_attr "type" "arith")])
+
+;; Old reload might generate add insns directly (not through the expander) for
+;; for the memory address of complex insns like atomic insns when reloading.
(define_insn_and_split "*addsi3"
+ [(set (match_operand:SI 0 "arith_reg_dest" "=r")
+ (plus:SI (match_operand:SI 1 "arith_reg_operand" "r")
+ (match_operand:SI 2 "const_int_operand" "rI08")))]
+ "TARGET_SH1 && !sh_lra_p ()
+ && reload_completed
+ && !reg_overlap_mentioned_p (operands[0], operands[1])"
+ "#"
+ "&& 1"
+ [(set (match_dup 0) (match_dup 1))
+ (set (match_dup 0) (plus:SI (match_dup 0) (match_dup 2)))])
+
+(define_insn_and_split "*addsi3"
[(set (match_operand:SI 0 "arith_reg_dest" "=r,r")
(plus:SI (match_operand:SI 1 "arith_reg_operand" "%0,r")
(match_operand:SI 2 "arith_operand" "rI08,Z")))]
I haven't read reload.*, but my speculation is that if something in reload
instantiates that rtx with an imm8 constant to calculate addresses, it might
also try to instantiate it with reg = reg + reg if the constant doesn't fit
into imm8. Maybe we just don't hit the case (yet). So it's probably safer to
allow reg = reg + reg, too.
> There are two "new" failures with gcc testsuite:
>
> FAIL: gcc.dg/atomic/c11-atomic-exec-4.c -Os (internal compiler error)
> FAIL: gcc.dg/atomic/c11-atomic-exec-4.c -Os (test for excess errors)
>
> which is not really new because these errors have popped up randomly
> in our testresults. After all, unfortunately, some SH synchronization
> insns are too complex for the old reload. You can avoid this PR and
> c11-atomic-exec-4.c ICE with -mlra.
I guess those "new" failures are R0 related?
