Alexandre, Thank you very much for the comments to http://codereview.chromium.org/549079. I think you should upload the changes to later http://codereview.chromium.org/543161 and continue 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 > at http://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 >> > > > A src/mips/stub-cache-mips.cc >> > > > A src/mips/constants-mips.h >> > > > A src/mips/test-mips.cc >> > > > A src/mips/simulator-mips.cc >> > > > A src/mips/test-interface-mips.h >> > > > A src/mips/assembler-mips.h >> > > > A src/mips/virtual-frame-mips.h >> > > > A src/mips/test-mips.h >> > > > A src/mips/simulator-mips.h >> > > > A src/mips/register-allocator-mips-inl.h >> > > > M src/jsregexp.cc >> > > > M src/execution.cc >> > > > M src/globals.h >> > > > M SConstruct >> > >> > > > On Jan 20, 6:18 pm, Alexandre Rames <[email protected]> >> wrote: >> > > > > Update: >> > > > > The code has been cleaned and tools/presubmit.py returns with no >> > > > > (significant) errors. >> > > > > So the only work left to do before another commit is to make the >> mips >> > > > > cctest tests work and check the simulator. >> > >> > > > > Alexandre >> > >> > > > > On Jan 20, 3:22 pm, Alexandre Rames <[email protected]> >> wrote: >> > >> > > > > > Hi, >> > >> > > > > > Could you give me some hint on how to run tests I implemented in >> test- >> > > > > > assembler-mips.cc ? >> > > > > > The file looks like this: >> > >> > > > > > #include ... >> > > > > > [...] >> > > > > > static void InitializeVM() { [...] } >> > > > > > #define __ assm. >> > >> > > > > > TEST(MIPS0) { >> > > > > > InitializeVM(); >> > > > > > v8::HandleScope scope; >> > > > > > Assembler assm(NULL, 0); >> > >> > > > > > // addition >> > > > > > __ addu(v0, a0, Operand(a1)); >> > > > > > __ jr(ra); >> > > > > > __ nop(); >> > >> > > > > > CodeDesc desc; >> > > > > > assm.GetCode(&desc); >> > > > > > [...] >> > > > > > CHECK_EQ(0xfffffffb, res); >> > >> > > > > > } >> > >> > > > > > [...] >> > >> > > > > > Thanks, >> > >> > > > > > Alexandre >> > >> > > > > > On Jan 20, 2:48 pm, Alexandre Rames <[email protected]> >> wrote: >> > >> > > > > > > Hi, >> > >> > > > > > > Thanks for reviewing my code. >> > > > > > > Here are some details and answers on several points. >> > >> > > > > > > About the contributor licence agreement: I have given it to my >> boss. >> > > > > > > It will be sent quite soon. >> > >> > > > > > > I have corrected everything you told, except one issue I >> detail >> > > > below. >> > > > > > > I hope the fixes will be enough to have it work on a 64bit >> Linux. I >> > > > > > > don't have one so please let me know of other future errors. >> > >> > > > > > > I worked on the simulator, the disassembler, and the debugger. >> (The >> > > > > > > arm code was really helpful. I mainly adapted it.) They all >> work, but >> > > > > > > the simulator still needs a few tests. However most >> instructions seem >> > > > > > > to work fine. I am currently working on some tests like the >> ones in >> > > > > > > test-assembler-<arch>.cc and test-disasm-<arch>.cc . This >> should be >> > > > > > > done today. Should I commit the code when the tests are >> implemented? >> > > > > > > There may still be a few mistakes, but there would at least be >> a good >> > > > > > > basis to work on. >> > >> > > > > > > About the issue I did not change: >> > > > > > > The concerned review urls are >> > > > > > > >> http://codereview.chromium.org/549079/diff/1/11#newcode153 >> > > > > > > and >> http://codereview.chromium.org/549079/diff/1/11#newcode153 >> > >> > > > > > > I had to use functions with such template when it uses >> conditional >> > > > > > > execution. >> > > > > > > On MIPS there is 'cmp' instruction or conditional execution >> like on >> > > > > > > ARM. MIPS assembly has instead contional branch instructions >> like >> > > > "beq >> > > > > > > r1 r2 offset". >> > > > > > > In MIPS CodeGenerator::Branch function s5 and s6 registers are >> loaded >> > > > > > > with the values to be compared and the condition is a property >> of the >> > > > > > > CodeGenerator. I do this because when generating code >> comparisons are >> > > > > > > done before their result is evaluated to branch. >> > > > > > > However using this kind of solution for other functions would >> require >> > > > > > > to setup s5 and s6 every time we want to compare something. >> That >> > > > would >> > > > > > > be really unefficient. >> > > > > > > I can't see another simple way to pass the arguments to >> functions >> > > > like >> > > > > > > DeferredCodeBranch(). >> > > > > > > We could get rid of the preprocessor #ifdef by using the mips >> > > > template >> > > > > > > for other architectures, which would then not use the extra >> > > > arguments, >> > > > > > > but that would be ugly... >> > >> > > > > > > Feel free to ask more details if you need. >> > >> > > > > > > Alexandre >> > >> > > > -- >> > > > 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
