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

open
None
5
2012-06-10
2012-05-04
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

  • Molnár Károly

    Molnár Károly - 2012-05-04

    The examples.tar.bz2 contains some examples, which presents capabilities of the optimization.

     
  • 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
     
  • Molnár Károly

    Molnár Károly - 2012-05-05

    "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.)

     
  • Molnár Károly

    Molnár Károly - 2012-05-07

    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.

     
  • Molnár Károly

    Molnár Károly - 2012-05-07

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

     
  • Molnár Károly

    Molnár Károly - 2012-05-07

    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. :-)

     
  • Molnár Károly

    Molnár Károly - 2012-06-05

    I do not know that we the habit among you. Difficult to understand (to rewrite) my code, or you just wait for the missing part? I'm waiting to get into the svn what so far I sent it to you. To cart poorly?

     
  • Raphael Neider

    Raphael Neider - 2012-06-10

    Review comments to about the first half of new code lines.

     
  • Raphael Neider

    Raphael Neider - 2012-06-10
    • assigned_to: nobody --> tecodev
     
  • Raphael Neider

    Raphael Neider - 2012-06-10

    I am in the middle of reviewing the code and have found some problematic passages, see attached file review-1.txt. I do not want to get answers to my questions, I just want you to have another look at the code. I am currently at pcode.c:12100, the changes extend until pcode.c:16200, so there is a good deal more to review.

    From my current impression, the patch re-computes a lot of liveness and def-use information on the pcode level, which is often similar to what the --optimize-df stuff does as well. The redundancy of code seems to be greatly enhanced by this patch. Though I do not like that very much, I have not yet looked at the effects of the patch with respect to code size. Since the patch addresses a large number of specific code sequences, one wonders if the use of some kind of peep-hole optimizer dialect would not have been a better approach.

    Anyway, assuming that the generated code size does in fact reduce significantly using this patch, I expect to accept the patch *as it is now* once the review is over. Should you want to change/update/fix the code, please do so only after this patch has made it to the svn.

    Regarding your previous post:
    Your code is not too difficult to understand, it's just a lot of code (3k lines reviewed so far, 4k to go). I'm not waiting for more code -- what I have is enough for now. So far, nothing seems to be missing.

    > To cart poorly?

    I do not understand this.

    Best regards
    Raphael

     
  • Molnár Károly

    Molnár Károly - 2012-06-12

    >> To cart poorly?

    > I do not understand this.

    Instead, we should have been to ask: I'm impatient? :-)

     

Log in to post a comment.