[Bug rtl-optimization/66156] New: [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[Bug rtl-optimization/66156] New: [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[ronald.wahl at raritan dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66156

            Bug ID: 66156
           Summary: [msp430] wrong code generated with -O2 -mlarge (zero
                    extension HI -> SI)
           Product: gcc
           Version: 6.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ronald.wahl at raritan dot com
  Target Milestone: ---

Created attachment 35544
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35544&action=edit
C file that is miscompiled

The attached C file is miscompiled by msp-gcc-elf when compiled with
"-mlarge -O2". I see this problem with the current source bundle from TI
as well as current gcc trunk.

The problem is that R13 (containing "val") is prematurely
cleared by a zero_extendhisi2 insn:

; R12 contains pointer to "array"
; R13 contains "val"
        PUSHM.A #1, R10
        PUSHM.A #3, R8
        ; end of prologue
        MOVA    R12, R8
        CMPX.W  #0, &g_flag { JEQ       .L2
        MOVX    &g_second, R12
        ADDA    R12, R12
        ADDA    R8, R12
        MOV.W   @R12, R12

        MOV.W   #0,R13 ; <---- R13 clobbered (by zero_extendhisi2 (insn 12))

        MOV.W   R12, R10
        MOV.W   R13, R11
        MOVX.W  &g_sum, R14
        MOVX.W  &g_sum+2, R15
        SUBX    R10, R14 { SUBCX        R11, R15
.L3:
        MOV.W   R13, R6 ; <---- R13 used after it was clobbered

        MOV.W   #0,R7
        MOV.W   R6, R10
        MOV.W   R7, R11
.L4:
        MOVX.W  #1, &g_flag
        ADD     R10, R14 ; cy
        ADDC    R11, R15
        MOVX    &g_first, R12
        ADDA    R12, R12
        ADDA    R8, R12

        MOV.W   R13, @R12 ; <---- R13 used after it was clobbered

        CMPX.W  #0, &g_flag { JEQ       .L4
        MOVX.W  R14, &g_sum
        MOVX.W  R15, &g_sum+2
        ; start of epilogue
        POPM.A  #3, r8
        POPM.A  #1, r10
        RETA
.L2:
        MOVX.W  &g_sum, R14
        MOVX.W  &g_sum+2, R15
        BRA     #.L3

In msp430.md there is a zero_extendhisi2 isnsn defined as follows:

  ;; Look for cases where integer/pointer conversions are suboptimal due
  ;; to missing patterns, despite us not having opcodes for these
  ;; patterns.  Doing these manually allows for alternate optimization
  ;; paths.
  (define_insn "zero_extendhisi2"
    [(set (match_operand:SI 0 "nonimmediate_operand" "=rm")
          (zero_extend:SI (match_operand:HI 1 "nonimmediate_operand" "0")))]
    "msp430x"
    "MOV.W\t#0,%H0"
  )

The purpose seems to be in-place zero-extending an operand.

The problem happens during reload right after IRA. During IRA insn 12
(zero_extendhisi2) has r42 as input and r43 as output. Furthermore r39
contains "val". IRA assigns:
r39 -> r13 (HI)
r42 -> r12 (HI)
r43 -> r10 (SI)

But during reload the "zero_extendhisi2" insn defined in msp430.md matches
and zero extends R12 (i.e. clears R13) instead of R10.

Commenting out the insn in msp430.md or adding reload_completed constraint
fixes the issue but may raise the issues again why this insns was defined -
whatever these are.

So what's going on here?

[Bug rtl-optimization/66156] [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[nickc at redhat dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66156

Nick Clifton <nickc at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nickc at redhat dot com

--- Comment #1 from Nick Clifton <nickc at redhat dot com> ---
Hi Ronald,

  What's going on is that this is a reload bug.  Reload wants to extend r42
into r43 (or rather r12 into r10) and it is assuming that the zero_extendhisi
pattern will do this.  Unfortunately it does not, it only extends in place, and
so an extra move is required to make the conversion complete.  Reload generates
this extra move, but of course it is too late, r13 has already been clobbered.

  Not being a reload guru, I have decided to take the easy way out and extend
the zero_extendhisi2 pattern so that it can cope with moving the value whilst
extending it.  This works, although it does still leave a potential bug in
reload for other targets.  But for now it is the simplest solution.  So I am
going to apply this patch to the msp430 sources.  Please let me know if you
have any problems with it.

Cheers
  Nick

Index: gcc/config/msp430/msp430.md
===================================================================
--- gcc/config/msp430/msp430.md (revision 223348)
+++ gcc/config/msp430/msp430.md (working copy)
@@ -588,10 +588,12 @@
 ;; patterns.  Doing these manually allows for alternate optimization
 ;; paths.
 (define_insn "zero_extendhisi2"
-  [(set (match_operand:SI 0 "nonimmediate_operand" "=rm")
-       (zero_extend:SI (match_operand:HI 1 "nonimmediate_operand" "0")))]
+  [(set (match_operand:SI 0 "nonimmediate_operand" "=rm,r")
+       (zero_extend:SI (match_operand:HI 1 "nonimmediate_operand" "0,r")))]
   "msp430x"
-  "MOV.W\t#0,%H0"
+  "@
+  MOV.W\t#0,%H0
+  MOV.W\t%1,%L0 { MOV.W\t#0,%H0"
 )

 (define_insn "zero_extendhisipsi2"

[Bug rtl-optimization/66156] [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[nickc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66156

--- Comment #2 from Nick Clifton <nickc at gcc dot gnu.org> ---
Author: nickc
Date: Tue May 19 11:42:44 2015
New Revision: 223354

URL: https://gcc.gnu.org/viewcvs?rev=223354&root=gcc&view=rev
Log:
        PR target/66156
        * config/msp430/msp430.md (zero_extendhisi2): Add support for
        separate source and destination registers.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/msp430/msp430.md

[Bug rtl-optimization/66156] [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[ronald.wahl at raritan dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66156

--- Comment #3 from Ronald Wahl <ronald.wahl at raritan dot com> ---
(In reply to Nick Clifton from comment #1)
> Hi Ronald,
> 
>   What's going on is that this is a reload bug.  Reload wants to extend r42
> into r43 (or rather r12 into r10) and it is assuming that the
> zero_extendhisi pattern will do this.  Unfortunately it does not, it only
> extends in place, and so an extra move is required to make the conversion
> complete.  Reload generates this extra move, but of course it is too late,
> r13 has already been clobbered.
> 
>   Not being a reload guru, I have decided to take the easy way out and
> extend the zero_extendhisi2 pattern so that it can cope with moving the
> value whilst extending it.  This works, although it does still leave a
> potential bug in reload for other targets.  But for now it is the simplest
> solution.  So I am going to apply this patch to the msp430 sources.  Please
> let me know if you have any problems with it.

Looks good. Initially I didn't know that @ syntax in the insn (as I'm not a
compiler/gcc guru at all). Now it's clear to me so I have actually learned
something. :-)

Anyway, at the moment the compiler generates

MOV.W   @R12, R12
MOV.W   R12,R10 { MOV.W #0,R11

without using R12 afterwards so if it would generate

MOV.W   @R12, R10
MOV.W   MOV.W #0,R11

it would be even better. But this is just nitpicking. Maybe the LRA backend you
are working on will do a better job later. ;-)

- ron

[Bug rtl-optimization/66156] [msp430] wrong code generated with -O2 -mlarge (zero extension HI -> SI)

[jozefl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66156

jozefl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
                 CC|                            |jozefl at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #4 from jozefl at gcc dot gnu.org ---
Fixed since GCC 6.