That's great ! Thank you Søren for correcting the changes from Mads' review.
Regard, Alexandre 2010/2/4 Søren Gjesse <[email protected]> > The first part of your MIPS work has now been committed as r3799. Thank you > very much, and keep up the good work. > > Regards, > Søren > > > On Wed, Jan 27, 2010 at 16:50, Søren Gjesse <[email protected]> wrote: > >> Hi Alexandre, >> >> It cool, that you have worked you way through all of CodeGen, even though >> it is not working yet. With that work done I think you should forget about >> FullCodeGen for now and focus on getting the CodeGen working. I suggest that >> you disable the native files, and start by being able to compile, call and >> return from the following JavaScript function: >> >> function f(){ return 42; } >> >> and then perhaps move to something like >> >> function f(){ return 40 + 2; } >> >> I will continue to look at your change adding the assembler, disassembler >> and simulator together with one or two tests of the assembler. You can take >> a look at test-assembler-ia32.cc for some simple assembler tests. >> >> Regarding the copyright it's not necessary for your copyright to be in >> the files for the work to be protected under copyright. We very much try to >> avoid having lists of names/companies in the copyright notices. However, as >> a contributer you will of cause get your name and email in the AUTHORS file. >> If Sigma Designs signs the corporate CLA ( >> http://code.google.com/legal/corporate-cla-v1.0.html) we can put "Sigma >> Designs Inc." in the AUTHORS file as well. >> >> By the way do you have a reference to the ABI specification (C/C++ calling >> conventions) for the (Linux?) platform running on the hardware you are >> currently targeting? >> >> Regards, >> Søren >> >> On Tue, Jan 26, 2010 at 02:18, Alexandre Rames <[email protected] >> > wrote: >> >>> Hi, >>> >>> I forgot to say the CLA has been completed and sent. I guess you >>> should get it soon. >>> Also I wanted to know about the headers and copyrights for mips files. >>> Can I add a line like "Copyright (c) Sigma Designs." or maybe a >>> mention like this for files with Sun copyright: "The original source >>> code has been modified significantly modified by Sigma Designs." >>> >>> Thanks, >>> >>> Alexandre >>> >>> On Jan 25, 4:27 pm, Alexandre Rames <[email protected]> wrote: >>> > Hi Søren, >>> > >>> > I uploaded a new version of the code. >>> > >>> > About code generation, I have actually already implemented much of the >>> > CodeGenerator. I used the mainly the ARM code and also a little of the >>> > x86 code to help me implement it for MIPS. However I did not look at >>> > the FullCodeGen yet. >>> > However at the end I could not manage to have everything work >>> > properly. Committing everything bits by bits is good way to check >>> > everyting again and have external comments on my code. >>> > >>> > I was trying to have the sample shell work. I passed the InstallNatives >>> > () (with default natives files) in bootstrapper.cc line 1550, but >>> > failed to setup custom C functions in ConfigureGlobalObjects(). >>> > >>> > I think the next step, outside from correcting the current code and >>> > style errors, should be to add more assembler tests, and maybe >>> > generate some simple code for integers operations. >>> > >>> > I'll start with more assembler tests, and wait for your opinion on >>> > what to implement next. >>> > >>> > Regards, >>> > >>> > Alexandre >>> > >>> > On Jan 25, 6:08 am, Søren Gjesse <[email protected]> wrote: >>> > >>> > >>> > >>> > > Alexandre, >>> > >>> > > Thank you very much for the comments tohttp:// >>> codereview.chromium.org/549079. I think you should upload the changes >>> > > to laterhttp://codereview.chromium.org/543161andcontinue the code >>> review >>> > > there. >>> > >>> > > Returning to code generation, the suggestion to go for implementing >>> the >>> > > FullCodeGen might not be the best way to go after all. As I indicated >>> it is >>> > > a moving target, and you would probably run into changes that >>> conflicts with >>> > > what you are doing. Taking the CodeGen instead, without using the >>> virtual >>> > > frame and register allocation could be an easier way forward. CodeGen >>> will >>> > > not change much, and have been a solid backend from the initial V8 >>> version. >>> > > Without the virtual frame and register allocation you should have the >>> stack >>> > > manipulation explicitly in the generated code and handle branching >>> using >>> > > plain labels and not JumpTarget. The ARM port is actually a bit like >>> this >>> > > already. It has hooks into the virtual frame, but does not use it >>> that much, >>> > > and currently there is no register allocation for ARM. >>> > >>> > > Regards, >>> > > Søren >>> > >>> > > On Fri, Jan 22, 2010 at 21:23, Søren Gjesse <[email protected]> >>> wrote: >>> > > > Alexandre, >>> > >>> > > > Good that gcl is now working for you again. I have reviewed the >>> change a >>> > > > bit further, but still mostly general build related comments and >>> style nits. >>> > > > After we have these basic things sorted we can move on to the real >>> substance >>> > > > of the change. >>> > >>> > > > Also I want to give you a little description of the review process. >>> First >>> > > > of all on your first uploaded change ( >>> > > >http://codereview.chromium.org/549079) it would be nice if you >>> could >>> > > > respond to the individual comments to indicate how you have handled >>> it. On >>> > > > your new change I can see that you have addressed some comments and >>> not >>> > > > others. The normal workflow is to address the comments, and after >>> that >>> > > > update the change so that the reviewers can se the changes. This >>> way there >>> > > > can be several iterations on the same change. You can see my latest >>> change >>> > > > athttp://codereview.chromium.org/545151. It has two patch sets, >>> the first >>> > > > being my initial change together with review comments and my >>> responses. The >>> > > > second patch set is after my changes addressing the comments and in >>> there >>> > > > one can see the difference between the patch sets. >>> > >>> > > > To add a new patch is just a matter of running 'gcl upload ...' >>> again. You >>> > > > should note that this works best if the same svn base revision is >>> used. If >>> > > > 'svn up' is run and a new patch set uploaded it can get a bit >>> confusing. I >>> > > > think we should iterate a bit on your new change, but at some point >>> you >>> > > > should move from r3220 to HEAD, and then start a new change. The >>> change >>> > > > needs to fit to HEAD to be committed. >>> > >>> > > > We have also talked a bit about what would be the easiest way for >>> you to >>> > > > start generating code from the AST. The situation is the following: >>> > > > Currently there is two code generators the CodeGen (in files >>> codegen-xxx.*) >>> > > > and the FastCodeGen (in files fast-codegen-xxx.* - which was >>> renamed to >>> > > > FullCodeGen in r3660 and r3662). CodeGen is the original code >>> generator >>> > > > which have evolved from V8 version 1.0 to its current state with >>> the virtual >>> > > > frame handling and lots of optimizations. The FastCodeGen was >>> introduced >>> > > > mainly to speedup loading of web pages. The fast in its name means >>> generate >>> > > > code fast and don't do so many optimizations. The reason for this >>> was that >>> > > > all the optimizations added to CodeGen made the code itself run >>> faster but >>> > > > the code generation became slower. Currently FastCodeGen is used >>> for most >>> > > > top-level code. Some JavaScript constructs are not supported by >>> FastCodeGen >>> > > > and code generation is then handled by CodeGen. However the rename >>> to >>> > > > FullCodeGen is to indicate that is will support all JavaScript >>> constructs, >>> > > > and we are quite close. The current CodeGen is then supposed to be >>> phased >>> > > > out and replaced by a new optimizing compiler which might not >>> support all >>> > > > JavaScript constructs but generate highly optimized code for the >>> one it >>> > > > does. FullCodeGen will be used for the rest. >>> > >>> > > > As you can see code generation in V8 is a moving target, and plans >>> might >>> > > > still change. Even so our suggestion is that you start with looking >>> at >>> > > > implementing FullCodeGen, as it is simpler than CodeGen. That will >>> probably >>> > > > be the fastest path to generating code for the empty function, >>> executing it >>> > > > and getting undefined back. From there you can add constructs until >>> all of >>> > > > JavaScript is supported and you can build V8 with the builtin >>> JavaScript and >>> > > > pass all the tests. As work is still in progress on the FullCodeGen >>> you >>> > > > might encounter some setbacks, but hopefully not unreasonable ones. >>> > >>> > > > Regards, >>> > > > Søren >>> > >>> > > > On Fri, Jan 22, 2010 at 00:56, Alexandre Rames < >>> [email protected]>wrote: >>> > >>> > > >> It was indeed a conflict between mercurial and svn. >>> > > >> I uploaded the changes. I'm still working on the simulator, but it >>> can >>> > > >> already be used. >>> > >>> > > >> Alexandre >>> > >>> > > >> On Jan 21, 2:15 pm, Alexandre Rames <[email protected]> >>> wrote: >>> > > >> > Oh! >>> > > >> > It might be because I also use a mercurial repository in the >>> same >>> > > >> > folder to keep track of my work. I did not think of this. >>> > > >> > I'll check that. >>> > >>> > > >> > Thanks, >>> > >>> > > >> > Alexandre >>> > >>> > > >> > On Jan 21, 1:54 pm, Søren Gjesse <[email protected]> wrote: >>> > >>> > > >> > > That looks quite strange. It looks as if gcl tries to use the >>> 'hg' >>> > > >> > > (Mercurial Distributed SCM) command. If you look at >>> > >>> > > >> > > >>> http://groups.google.com/group/chromium-dev/browse_thread/thread/e775. >>> > > >> .. >>> > >>> > > >> > > I would have expected to see 'svn' in place of 'hg' in the >>> error >>> > > >> message. >>> > >>> > > >> > > There is a few mentions of Mercurial on the mailing list >>> > >>> > > >> > > >>> http://groups.google.com/group/chromium-dev/browse_thread/thread/affc. >>> > > >> .. >>> > >>> > > >> > > I will suggest that you take a look at GuessVCS/GuessVCSName >>> in >>> > > >> > > depot_tools/upload.py, to figure out what is happening. >>> > >>> > > >> > > Regards, >>> > > >> > > Søren >>> > >>> > > >> > > On Thu, Jan 21, 2010 at 20:25, Alexandre Rames < >>> > > >> [email protected]>wrote: >>> > >>> > > >> > > > I'm sorry but I don't manage to upload my changes... >>> > > >> > > > I created a fresh checkout of revision 3220, added mips >>> files and >>> > > >> > > > modified the other. >>> > > >> > > > gcl opened seems to indicate everything is fine, but when I >>> try to >>> > > >> > > > upload I get this error: >>> > >>> > > >> > > > `--> gcl upload 2ndMIPSCommit >>> > > >> > > > Got error status from ['hg', 'cat', '-r', 'cd8114d1dfc6', >>> > > >> 'test/cctest/ >>> > > >> > > > test-assembler-mips.cc']: >>> > >>> > > >> > > > I tried to remove test-assembler-mips.cc but it did not >>> help. Any >>> > > >> > > > idea? >>> > >>> > > >> > > > gcl opened outputs the following: >>> > > >> > > > `--> gcl opened >>> > > >> > > > --- Changelist 2ndMIPSCommit: >>> > > >> > > > A test/cctest/test-assembler-mips.cc >>> > > >> > > > M src/spaces-inl.h >>> > > >> > > > M src/register-allocator.h >>> > > >> > > > M src/heap.cc >>> > > >> > > > M src/codegen-inl.h >>> > > >> > > > M src/bootstrapper.cc >>> > > >> > > > M src/flag-definitions.h >>> > > >> > > > M src/jump-target.cc >>> > > >> > > > M src/regexp-macro-assembler.cc >>> > > >> > > > M src/codegen.h >>> > > >> > > > M src/runtime.js >>> > > >> > > > M src/virtual-frame.h >>> > > >> > > > M src/objects.h >>> > > >> > > > M src/SConscript >>> > > >> > > > M src/macro-assembler.h >>> > > >> > > > M src/assembler.h >>> > > >> > > > M src/v8.cc >>> > > >> > > > M src/platform-linux.cc >>> > > >> > > > M src/frames-inl.h >>> > > >> > > > M src/objects-inl.h >>> > > >> > > > M src/jump-target.h >>> > > >> > > > M src/code-stubs.h >>> > > >> > > > M src/heap.h >>> > > >> > > > M src/register-allocator-inl.h >>> > > >> > > > A src/mips >>> > > >> > > > A src/mips/codegen-mips-inl.h >>> > > >> > > > A src/mips/ic-mips.cc >>> > > >> > > > A src/mips/assembler-mips-inl.h >>> > > >> > > > A src/mips/jump-target-mips.cc >>> > > >> > > > A src/mips/register-allocator-mips.cc >>> > > >> > > > A src/mips/codegen-mips.cc >>> > > >> > > > A src/mips/regexp-macro-assembler-mips.h >>> > > >> > > > A src/mips/constants-mips.cc >>> > > >> > > > A src/mips/frames-mips.cc >>> > > >> > > > A src/mips/virtual-frame-mips.cc >>> > > >> > > > A src/mips/macro-assembler-mips.h >>> > > >> > > > A src/mips/frames-mips.h >>> > > >> > > > A src/mips/disasm-mips.cc >>> > > >> > > > A src/mips/debug-mips.cc >>> > > >> > > > A src/mips/cpu-mips.cc >>> > > >> > > > A src/mips/builtins-mips.cc >>> > > >> > > > A src/mips/fast-codegen-mips.cc >>> > > >> > > > A src/mips/readme >>> > > >> > > > A src/mips/register-allocator-mips.h >>> > > >> > > > A src/mips/regexp-macro-assembler-mips.cc >>> > > >> > > > A src/mips/codegen-mips.h >>> > > >> > > > A src/mips/macro-assembler-mips.cc >>> > > >> > > > A src/mips/assembler-mips.cc >>> > > >> > > > A src/mips/test-interface-mips.cc >>> > >>> > ... >>> > >>> > read more » >>> >>> -- >>> v8-users mailing list >>> [email protected] >>> http://groups.google.com/group/v8-users >>> >> >> > -- > v8-users mailing list > [email protected] > http://groups.google.com/group/v8-users > -- v8-users mailing list [email protected] http://groups.google.com/group/v8-users
