[Bug target/64160] New: msp430 code generation error adding 32-bit integers

[Bug target/64160] New: msp430 code generation error adding 32-bit integers

[pab at pabigot dot com]

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

            Bug ID: 64160
           Summary: msp430 code generation error adding 32-bit integers
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pab at pabigot dot com

This program:

struct adjustment {
  unsigned long duration;
};
unsigned long read_clock ();
int dosomething (struct adjustment * ap, unsigned long v);
int doit (struct adjustment * ap)
{
  return dosomething(ap, read_clock() + ap->duration);
}

when compiled with msp430-elf-gcc -Os -S produces:

       ; start of prologue
        PUSHM.W #1, R10
        ; end of prologue
        MOV.W   R12, R10
        CALL    #read_clock
        MOV.W   @R10, R13    <<-- error half of return value clobbered
        ADD     R12, R13 ; cy
        MOV.W   2(R10), R14
        ADDC    R13, R14     <<-- R13 is not what it should be
        MOV.W   R10, R12
        CALL    #dosomething

which produces the wrong answer because the upper word of the result from
read_clock was overwritten by the read from memory.

[Bug target/64160] msp430 code generation error adding 32-bit integers

[uweigand at gcc dot gnu.org]

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

Ulrich Weigand <uweigand at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |uweigand at gcc dot gnu.org

--- Comment #1 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
This is not a reload problem, it's a bug in the addsi3 splitter where it
doesn't account for overlapping source and destination.  We have initially:

(insn 11 10 12 2 (set (reg:SI 13 R13)
        (plus:SI (reg:SI 12 R12)
            (mem:SI (reg/v/f:HI 28 [ ap ]) [2 ap_4(D)->duration+0 S4 A16])))
bug.i:10 20 {addsi3}
     (expr_list:REG_DEAD (reg:HI 12 R12)
        (nil)))

Note where the destination (reg:SI 13) partially overlaps the source (reg:SI
12).

After split 1 we have:

(insn 22 10 23 2 (parallel [
            (set (reg:HI 13 R13)
                (plus:HI (reg:HI 12 R12)
                    (mem:HI (reg/v/f:HI 28 [ ap ]) [2 ap_4(D)->duration+0 S2
A16])))
            (set (reg:BI 2 R2)
                (truncate:BI (lshiftrt:SI (plus:SI (zero_extend:SI (reg:HI 12
R12))
                            (zero_extend:SI (mem:HI (reg/v/f:HI 28 [ ap ]) [2
ap_4(D)->duration+0 S2 A16])))
                        (const_int 16 [0x10]))))
        ]) bug.i:10 -1
     (nil))
(insn 23 22 12 2 (set (reg:HI 14 R14 [+2 ])
        (plus:HI (plus:HI (reg:HI 13 R13 [+2 ])
                (mem:HI (plus:HI (reg/v/f:HI 28 [ ap ])
                        (const_int 2 [0x2])) [2 ap_4(D)->duration+2 S2 A16]))
            (zero_extend:HI (reg:BI 2 R2)))) bug.i:10 -1
     (nil))

Note how the first insn of the pair now clobbers the part of the source that is
still needed for the second half.

[Bug target/64160] msp430 code generation error adding 32-bit integers

[nickc at redhat dot com]

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

Nick Clifton <nickc at redhat dot com> changed:

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

--- Comment #2 from Nick Clifton <nickc at redhat dot com> ---
Created attachment 34232
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34232&action=edit
Proposed patch

Hi Peter,

  Please try out this patch and let me know what you think.

Cheers
  Nick

[Bug target/64160] msp430 code generation error adding 32-bit integers

[pab at pabigot dot com]

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

--- Comment #3 from Peter A. Bigot <pab at pabigot dot com> ---
Comment on attachment 34232
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34232
Proposed patch

I don't trust that the term nonsubreg is being used correctly in that predicate
since the operand does have subregs (there's no documentation of what that
predicate is intended to recognize).  I'm also unconvinced that the test
captures all the ways the operands might overlap: it seems one-sided, and
ignores operand[2], though I'm not going to try to find a counter-example.

I concede it fixes this specific example, but it doesn't leave me confident in
the validity of the split.

[Bug target/64160] msp430 code generation error adding 32-bit integers

[uweigand at gcc dot gnu.org]

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

--- Comment #4 from Ulrich Weigand <uweigand at gcc dot gnu.org> ---
The ususal test in such scenarios involves reg_overlap_mentioned_p:

/* Nonzero if modifying X will affect IN.  [...]  */
int
reg_overlap_mentioned_p (const_rtx x, const_rtx in)

which also handles cases like where the modified register is used as a base
register in a MEM in the second operand (not sure whether this can happen on
your platform).

A condition along the lines of

 "!reg_overlap_mentioned_p (msp430_subreg (HImode, operands[0], SImode, 0),
                            msp430_subreg (HImode, operands[1], SImode, 2))
  && !reg_overlap_mentioned_p (msp430_subreg (HImode, operands[0], SImode, 0),
                               msp430_subreg (HImode, operands[2], SImode, 2))"

should probably catch all invalid cases (i.e. if modifying op3 will affect op7
and/or op8, the split is invalid).

Or, to avoid the duplicate msp430_subreg computation, you might simply instead
add a FAIL at the end of the split instructions:

  if (reg_overlap_mentioned_p (operands[3], operands[7])
      || reg_overlap_mentioned_p (operands[3], operands[8]))
    FAIL;

B.t.w. is there a particular reason why the target-specific msp430_subreg is
needed instead of the usual operand_subword?

As to your predicate question, msp430_nonsubreg_operand is defined as:
(define_predicate "msp430_nonsubreg_operand"
  (match_code "reg,mem"))
so it is true if and only if the operand is a REG or a MEM, which means it
would indeed reject SUBREGs.  (Of course a register operand can still *have*
subregs, it just cannot itself *be* a subreg.)

Again, it's not completely clear to me why this special predicate is needed in
the first place; it seems to be solely used in this one splitter.  (Maybe this
is related to some restriction in msp430_subreg, which goes back to the
question what *that* is needed instead of operand_subword, which ought to
handle all general-operand cases.)

[Bug target/64160] msp430 code generation error adding 32-bit integers

[pab at pabigot dot com]

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

--- Comment #5 from Peter A. Bigot <pab at pabigot dot com> ---
Thanks, Ulrich: that's a better explanation of what makes me uncomfortable with
that part of the machine description.

We have a complex split pattern that's the sole user of an idiosyncratic
predicate.  It supports a set of other patterns that are introduced with a
comment starting with "we are playing a dangerous game with GCC here" and going
on to suggest "we put in some hacks to fix the situations we found where it
didn't work".  It only works on SI, not QI (and yes people do use 64-bit
integer operations on this target).  It's only done for add, not subtract. 
It's not clear why subregs are to be excluded from the overall concept of
eliminating partial results.

Basically, the whole set of patterns exposing carry looks like an attempt to
optimize what's produced by a specific source-level expression, instead of a
carefully constructed transformation designed to match the capabilities of the
target ISA.

As MSP430 user I'd rather see it pulled and replaced with something simple that
works even if it isn't optimal: in mspgcc I just generated an ADD (SUB)
followed by a sequence of ADDC (SUBC) determined by operand size and that
worked out fine.  Fixing the inefficiencies described in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64027 would provide more value
than special-casing 32-bit add operations.

[Bug target/64160] msp430 code generation error adding 32-bit integers

[nickc at redhat dot com]

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

--- Comment #6 from Nick Clifton <nickc at redhat dot com> ---
Hi Ulrich,

>  if (reg_overlap_mentioned_p (operands[3], operands[7])
>      || reg_overlap_mentioned_p (operands[3], operands[8]))
>    FAIL;

Thanks - that is indeed a better solution to the bug.

> B.t.w. is there a particular reason why the target-specific msp430_subreg is
> needed instead of the usual operand_subword?


I am not sure.  According to the comments in msp430.c it is because;
"Simplify_gen_subreg() doesn't handle memory references the way we need it to
below, so we use this function for when we must get a valid subreg in a
'natural' state."  I think that this is all connected with the fact that for
the MSP430 in large mode, pointers are 20-bits long, and that this tends to
confuse gcc.

Cheers
  Nick

[Bug target/64160] msp430 code generation error adding 32-bit integers

[nickc at gcc dot gnu.org]

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

--- Comment #7 from Nick Clifton <nickc at gcc dot gnu.org> ---
Author: nickc
Date: Wed Dec 24 13:36:29 2014
New Revision: 219058

URL: https://gcc.gnu.org/viewcvs?rev=219058&root=gcc&view=rev
Log:
    PR target/64160
    * config/msp430/msp430.md (addsi splitter): Do not split when the
    destination partially overlaps the source.

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