diff mbox

RFA: Patch for ARM PR77770

Message ID 87ziifph6t.fsf@redhat.com
State New
Headers show

Commit Message

Nick Clifton Jan. 25, 2017, 10:28 a.m. UTC 
Hi Richard, Hi Ramana,

  The patch below is a simple fix for PR77770.  I am not really
  expecting you to agree with it, but I thought that it was worth
  posting so that this PR could be looked at again and maybe a better
  patch found.  (Plus I am trying to close PRs so that the gcc 7 branch
  will happen...)

  The patch is simple - it disparages the SF-mode store alternative in
  the thumb1_movsf_insn just enough to break the infinite load/store
  cycle being triggered in reload.  The patch does not introduce any
  regressions, although I suspect it might affect code quality for
  thumb1.  (I have not checked this).  Nor have I gone deep into why
  reload is generating an infinite cycle of SF load/store insns.  That
  was a bit beyond me.  But the patch works and maybe that is enough.

  Obviously, if you think that this patch is OK, I would also create a
  new ARM specific gcc testsuite entry based upon the simplified test in
  the PR.

  So - OK to apply ?
  
Cheers
  Nick

Comments

Richard Earnshaw (lists) Jan. 25, 2017, 11 a.m. UTC | #1 
On 25/01/17 10:28, Nick Clifton wrote:
> Hi Richard, Hi Ramana,
> 
>   The patch below is a simple fix for PR77770.  I am not really
>   expecting you to agree with it, but I thought that it was worth
>   posting so that this PR could be looked at again and maybe a better
>   patch found.  (Plus I am trying to close PRs so that the gcc 7 branch
>   will happen...)
> 
>   The patch is simple - it disparages the SF-mode store alternative in
>   the thumb1_movsf_insn just enough to break the infinite load/store
>   cycle being triggered in reload.  The patch does not introduce any
>   regressions, although I suspect it might affect code quality for
>   thumb1.  (I have not checked this).  Nor have I gone deep into why
>   reload is generating an infinite cycle of SF load/store insns.  That
>   was a bit beyond me.  But the patch works and maybe that is enough.
> 
>   Obviously, if you think that this patch is OK, I would also create a
>   new ARM specific gcc testsuite entry based upon the simplified test in
>   the PR.
> 
>   So - OK to apply ?

No, I don't think so.  At least not on trunk.

If, come the time of the release, we have no better solution it might be
ok to put something like this on the release branch as a palliative, but
I don't think we should just paper over the underlying problem.  It
could be that another testcase will still fail with this change.

R.

>   
> Cheers
>   Nick
> 
> Index: gcc/config/arm/thumb1.md
> ===================================================================
> --- gcc/config/arm/thumb1.md	(revision 244853)
> +++ gcc/config/arm/thumb1.md	(working copy)
> @@ -865,7 +865,7 @@
>     (set_attr "conds" "clob,nocond,nocond,nocond,nocond")])
>  ;;; ??? This should have alternatives for constants.
>  (define_insn "*thumb1_movsf_insn"
> -  [(set (match_operand:SF     0 "nonimmediate_operand" "=l,l,>,l, m,*r,*h")
> +  [(set (match_operand:SF     0 "nonimmediate_operand" "=l,l,>,l, ?m,*r,*h")
>  	(match_operand:SF     1 "general_operand"      "l, >,l,mF,l,*h,*r"))]
>    "TARGET_THUMB1
>     && (   register_operand (operands[0], SFmode)
>
diff mbox

Patch

Index: gcc/config/arm/thumb1.md
===================================================================
--- gcc/config/arm/thumb1.md	(revision 244853)
+++ gcc/config/arm/thumb1.md	(working copy)
@@ -865,7 +865,7 @@ 
    (set_attr "conds" "clob,nocond,nocond,nocond,nocond")])
 ;;; ??? This should have alternatives for constants.
 (define_insn "*thumb1_movsf_insn"
-  [(set (match_operand:SF     0 "nonimmediate_operand" "=l,l,>,l, m,*r,*h")
+  [(set (match_operand:SF     0 "nonimmediate_operand" "=l,l,>,l, ?m,*r,*h")
 	(match_operand:SF     1 "general_operand"      "l, >,l,mF,l,*h,*r"))]
   "TARGET_THUMB1
    && (   register_operand (operands[0], SFmode)