Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#178 New commandline option to the PIC16 branch: --optimize-hard

open
None
5
2012-06-10
2012-05-04
Molnár Károly
No

This patch implements a powerful optimization in the pcode.c module.
Simplifies the logic operations.
Where possible, use the FSR1 register pair the FSR2 instead.
Reduces the unnecessary use of temporary registers.
Optimize the data loading of the WREG.
In addition, performs many other optimization.

Discussion

1 2 > >> (Page 1 of 2)
  • The examples.tar.bz2 contains some examples, which presents capabilities of the optimization.

     
    Attachments
  • Raphael Neider
    Raphael Neider
    2012-05-05

    • assigned_to: nobody --> tecodev
     
  • Raphael Neider
    Raphael Neider
    2012-05-05

    Wow, this is a massive 15 K lines of patch (KLOP ;-)) ...
    Before I try to apply that one, let me comment of some of the changes:

    sdcc/src/pic16/genarith.c:1912
    MOVFF reg, WREG => MOVFW reg
    is more general and could better be done in an optimization step. Otherwise we end up special casing assignment to WREG througout the code. Alternatively one could use some mov2f() function that deals with WREG assignments properly and use mov2f() everywhere. Right now I'd prefer an optimization phase for this as it is the least intrusive solution.

    sdcc/src/pic16/gen.c:2180 (0 => reg, 0xff => reg)
    This seems to fail for (src->type == AOP_LIT) && dstIsWreg && ((lit == 0x00) || (lit == 0xff)), as there is no assignment to WREG at all? I doubt that WREG will actually ever be the destination here, but still ... I also think that this optimization would better be done later -- and IIRC actually is done in the data-flow-analysis-based optimization.

    This is duplicated in 2243ff and close enough in l. 2289ff and 2337ff and thus should probably be refactored into something like a movlit2reg() function.

    2507ff looks like a bugfix rather than an optization (though the buggy case probably never occurs: value in WREG being interpreted as bool ...).

    3703: here you check "operand is not WREG" using dest->type != PO_WREG, and three lines after that you use dest->type != PO_WREG && dest->type != PO_W ... Seems suspicious / incomplete. Generally, I do not fully trust the PO_-types (pure intuition, no evidence there).

    9322: see 1912: MOVFF reg, WREG => MOVFW reg

    sdcc/src/pic16/pcode.c
    4767ff: Is PCOI(pcop)->offset < 0 meaningful? Does your code do "the right thing" or "just" avoid segfaults by emitting valid (but possibly wrong) code?

    If I remember correctly, only __LINE__ and __FILE__ are required from CPP, __FUNCTION__ is a GNU extension and might fail on MSVC. __func__ might work on any recent C/C++ compiler, but might also be C99 if at all. So I would avoid __FUNCTION__ if it does not appear elsewhere in the source.

    Fixing whitespace/punctuation in output and comments as well as cosmetic changes such as removing redundant break; statemens is welcome, but should really not be intermingled with functional changes. A separate patch would have been/be better.

    5804: looks like a bug fix to me, which should also be filed separately (despite the code being commented right now).

    I'll have to review the 13890 lines added after l.7278 before committing them, but I doubt there is any use in trying to chop that block up into smaller chunks? These lines together add your optimizations, right?

    I'm not sure how to proceed here: Will you revise your patch or shall I start working on integrating some of your changes and risk merge conflicts?!? I'm putting this off again for now.

     
  • Raphael Neider
    Raphael Neider
    2012-05-05

    • assigned_to: tecodev --> nobody
     
  • "Wow, this is a massive 15 K lines of patch (KLOP ;-)) ..."
    Most of the code over one year old. (I suspected that many will be all at once.)

    "sdcc/src/pic16/genarith.c:1912"

    The whole point is:
    WREG = x;
    or
    WREG = x + y;
    The results of the operation is stored in WREG.
    If you are not good, then this section calmly leave out.

    "sdcc/src/pic16/pcode.c
    Is PCOI(pcop)->offset < 0 meaningful?"

    This change is needed to correct display the LFSR instruction. It seemed a simple solution.

    "
    If I remember correctly, only __LINE__ and __FILE__ are required from CPP,
    __FUNCTION__ is a GNU extension and might fail on MSVC."

    Windows? I had not thought of.

    "__func__ might work
    on any recent C/C++ compiler, but might also be C99 if at all. So I would
    avoid __FUNCTION__ if it does not appear elsewhere in the source.
    "
    If the gcc know this: __ func__. Then there is no obstacle to the exchange.

    "These lines together add your optimizations, right?"

    Together work with the whole, but can be broken down into pieces. In the beginning, only the "pic16_OptimizeHard" existed, and some auxiliary procedures.

    "I'm not sure how to proceed here"

    You decide what's right for you. I try to adapt. I just do not forget that I little understand English. The to translation of the translate.google.com I use. Often I get silly result. (Maybe decide to not worth the effort to deal with me.)

     
  • I made a smallest patch. I skipped some change. Also skipped some parts of the optimization. My question is: Can I upload the new patch in the "Attached Files" list? Or I open a new patch, and I send forth?

     
  • Raphael Neider
    Raphael Neider
    2012-05-07

    > Maybe decide to not worth the effort to deal with me.

    Definitely not, your contributions are welcome.

    > I made a smallest patch. I skipped some change.
    > Also skipped some parts of the optimization.

    Great. Feel free to post the remaining changes as well in separate patches (if at all possible with reasonable effort).

    > My question is: Can I upload the new patch in the "Attached Files" list? Or I open a new patch, and I send forth?

    You should be able to upload the patch/patches to this tracker item. The grey text "Add a file" in the section titled "Attached files" below is actually a link that reveals the upload form. If you want to upload multiple patches, it might be a good idea to collect them in a compressed ZIP or TAR archive.
    If you cannot upload your patches here, just open a new item. In this case, please add a comment here with the ID of the follow-up item for future reference.

     
  • This a piece of the sdcc-3.1.4-svn7661.patch.gz. Unfortunately, this is still more than 9000 line.

     
  • For now, I thought that so much enough. If this works well with you then I send the rest as well. I do not want to bury in the work. :-)

     
1 2 > >> (Page 1 of 2)