#353 Wrong optimization in postreload pass. (+fix)

4.6.x_development
open
nobody
None
5
2014-12-31
2013-05-09
Ronald Wahl
No

I have found an code optimization issue with gcc for msp430. Currently I use v4.6.3 with the LTS patch but the bug is probably in all variants (or actually in the non-arch specific part).

The issue is that my code is miscompiled because a mov instruction is removed which is required:

        .loc 4 54 0
        push.b  #40
.LCFI13:
        mov     #llo(23438215), r12
        mov     #lhi(23438215), r13
        mov.b   #1, r14
        mov     r1, r15           <--------- This one is removed in postreload
        add     #556, r15
.LCFI14:
        call    #_ZN13CurrentSensorC1Ehmh

The reason is that the pushqi1 insn uses "pre_modify" and move2add_note_store() in postreload.c does not handle it. So I propose the following patch (inspired by gcc PR/47008):

diff -uNr gcc-4.6.3.orig/gcc/postreload.c gcc-4.6.3/gcc/postreload.c
--- gcc-4.6.3.orig/gcc/postreload.c     2011-02-11 21:52:55.000000000 +0100
+++ gcc-4.6.3/gcc/postreload.c  2013-05-09 14:21:39.963390084 +0200
@@ -2091,7 +2091,8 @@
     {
       dst = XEXP (dst, 0);
       if (GET_CODE (dst) == PRE_INC || GET_CODE (dst) == POST_INC
-         || GET_CODE (dst) == PRE_DEC || GET_CODE (dst) == POST_DEC)
+         || GET_CODE (dst) == PRE_DEC || GET_CODE (dst) == POST_DEC
+         || GET_CODE (dst) == PRE_MODIFY || GET_CODE (dst) == POST_MODIFY)
        reg_set_luid[REGNO (XEXP (dst, 0))] = 0;
       return;
     }

Does this make sense? Dunno if this should be fixed upstream even if it is not yet triggered by any of the officially supported architectures.

Discussion

  • Peter A. Bigot

    Peter A. Bigot - 2013-05-09

    At a quick glance, it makes sense, and is worth filing as an upstream "obvious" bug given the existing code is not consistent with the similar test in reload_combine_note_store. It'll probably be ignored, but should still be reported.

    If you have a reproducing test case I'll consider it for any future patches to mspgcc, but since the Red Hat rewrite is being merged upstream it's not likely there will be more mspgcc patches.

     
  • Ronald Wahl

    Ronald Wahl - 2013-05-09

    Ok, I created an upstream bug report: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57229
    The rewrite seems to different so it might not trigger the bug. So if it is unlikely that you are going to create any new mspgcc patches then I skip the pain of creating a minimal testcase. I already spent too much time in debugging this as I'm not really a compiler respective gcc guru. But at least I learned a lot so it was not a real waste of time. :-)

     

Log in to post a comment.