[fpc-devel] Parallel processing in the compiler

[Jonas Maebe]

On 05 Sep 2010, at 05:04, Hans-Peter Diettrich wrote:

> Jonas Maebe schrieb:
>> I think it is quite systematic and logical that the ppu loading
>> should not depend on code generator internals.
> 
> Right, that's how it *should* be designed.

That's how it is in trunk.

> But try to find out why the code generation is added, when variables like current_filepos or current_tokenpos are moved into TModule (current_module) :-(

Nobody ever said that it would be easy to restructure the compiler. In fact, I will be the first to admit that it is incredibly hard, and since the compiler is 15 years old (although several parts have been rewritten over the years), there are bound to be quite a few less than ideal situations and things that simply grew over time. However, I very much dislike the way you are doing the development and find it very hard to see how we are ever going to be able to integrate any of those changes back into trunk.

Maybe I simply should stop being involved in this, since I'm obviously not able to productively deal with this project.

>> You have to describe in your svn commit logs what you actually do.
>> "NG: fixed make all" is not a useful commit message. You changed 149
>> files in that commit, and did not describe what was done nor why:
> 
> I only commited these changes so that they don't get lost.

You should never commit unrelated changes in a *single* commit. Even if you don't want to create a separate branch for other changes, at the very least use separate commits (although unrelated changes will make merging more difficult in case later changes also depend on them).

> Please review the preceding version, as mentioned in the Readme.

The main problem is that it cannot be merged since it doesn't compile. As I mentioned earlier: the ideal situation are piecemeal changes that form a tested and self-contained whole that can be easily reviewed and merged. While a single change related to global variables can obviously have repercussions all over the compiler, that does not mean that you have to replace all global variables that at first sight should end up in tscanner with fields in one go, especially if you are unable to completely test those changes (make all, make fullcycle, running the testsuite).

> The last change was to remove ppudump from the Makefile, and this proved that so far only ppudump is sensitive to changes in the compiler internals.

It also proves that the internal compiler structure has been broken.

>> * you
>> deleted the aasmsym unit and changed code that depended on it without
>> any remarks why you did that. The comment in the header of that unit
>> said: "Contains abstract assembler instructions for all processor types, including routines which depend on the symbol table. These
>> cannot be in aasmtai, because the symbol table units depend on that
>> one."
> 
> I wanted to make code generation more abstract, so that it can be separated better from the remaining code.

Removing an extra abstraction class and folding it into the base class is the opposite of making something more abstract.

> The GlobVar unit is intended as a place for all (previously) global variables, that can be used without importing further dependencies.

The aasmsym unit did not contain any global variables. And putting all global variables in a single unit may not be the right approach since the compiler consists of many components. They are obviously interwoven, but making them more interwoven by basically making all types of all global variables globally visible does not sound a like a good idea.

>> You nevertheless did move that code to aasmtai and now there are
>> indeed (implementation-level) circular dependencies between aasmtai
>> and symsym. Adding extra circular dependencies to the compiler
>> without any argument as to why this is required is not good.
> 
> I don't remember details.

That's why you should write it in the svn log. Note that you can edit the fpc repository's svn log entries even after you have committed, using "svn pe -r <revnr> --revprop svn:log".

> My experimental approach left these units unmodified, and still resulted in importing code generation (see above).

That's because of other changes. Making the problem worse does not contribute to the solution.

>> * that
>> commit also contains changes that have nothing to do with fixing
>> "make all", such as fixing a typo in a comment, removing type
>> redefinitions, commenting the assignment of tcgaddnode to caddnode in
>> the init code of ncgadd.pas (and similar assignments in ncgmat),
>> possibly an unrelated change in ppheap (which is wrongly indented),
>> removing an unused local type definition from ppu.pas...
> 
> I tried to follow the coding style, to not comment anything in the code.

Oh, please.

> Please suggest an better and more systematic way of reducing the number of compiler hints, and supply a justification for redefinitions of system unit types.

I'm not saying that the local type definitions should stay. I'm saying that removing them must not be committed together with completely unrelated changes.

>> * I assume
>> that all the code you added in psystem.pas is with the long term view
>> of making the parser independent. However, that is a separate project
>> and not part of removing global variables from the compiler
> 
> Both are related, since the "stateful" variables are related to current_module, and that's where the scanner and parser is involved, somehow.

That does not answer my remark, but I don't know how to make my point clearer.

>> While reviewing your previous commits, I noticed that you also
>> integrated your patch from
>> http://bugs.freepascal.org/view.php?id=16888 although that one has
>> nothing to do with removing the use of globals.
>> This way of working is not how we can keep the compiler development
>> manageable for multiple people to work on.
> 
> I added some improvements and corrected typos, while waiting for comments on my work. It would be nice to know in *advance*, what should be avoided.

* don't add cyclic dependencies: http://lists.freepascal.org/lists/fpc-devel/2010-July/021065.html
* commit unrelated changes separately (even to separate branches if possible), polish/finish one self-contained part at a time so it can be merged: http://lists.freepascal.org/lists/fpc-devel/2010-August/021558.html

I may indeed not have mentioned before that svn commit messages should explain the idea behind the changes and why some things are done in a certain way.

> The lack of response to my repeated review requests is not what I consider as "multiple people working on the same project".

There are more people that you alone, and many people have spent a lot of time on answering many questions from you. Reviewing a bunch of changes with little or no information about why which changes were done is
a) a lot of work
b) not very enticing

> In the meantime I started several projects, not commited to the repository, one of which (almost only) moves global variables into TModule, without breaking ppudump and affecting unrelated units. If you think that this is a better base for cooperation, I can try to add it as another branch, with no detailed history.

I don't know.


Jonas
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freepascal.org/pipermail/fpc-devel/attachments/20100905/9c65fc71/attachment.html>