Bug 599251 introduced the non-numeric Register type. This back-end doesn't use it yet. It should. See the i386/X64 back-ends for examples.

Comment on attachment 485873 [details] [diff] [review] fix asserts so we can compile Wow, NativeSH.cpp is absurdly verbose. Some factoring out of duplicated code would be welcome (but beyond the scope of this bug).

Not sure to understand what is verbose, I do not make comment for absurd. Do you mean that preconditions are useless? Do you mean that we should "refactor" by hand a set of instruction semantics that is automatically generated from a proven architecture description? I do not see where would be the benefit in doing so. I let Cedric comment and regenerate the description.

(In reply to comment #3) > Not sure to understand what is verbose, I do not make comment for absurd. > Do you mean that preconditions are useless? The code is incredibly repetitive. The attached patch is the start of a refactoring that would reduce the file size by 100+ lines. If that refactoring was fully applied, Rick's patch would only be a few lines rather than hundreds of lines. > Do you mean that we should "refactor" by hand a set of instruction semantics > that is automatically generated from a proven architecture description? > I do not see where would be the benefit in doing so. > > I let Cedric comment and regenerate the description. The code is automatically generated? I didn't know that. If you have auto-generated code in a repository you should also include the program that does the auto-generation. In fact, the auto-generated code itself probably should not be in the repository, because people will modify NativeSH4.cpp and that'll get overwritten next time you auto-generate. For example, Rick's changes in the attached patch would be overwritten. You could still modify your auto-generation program to do the above refactoring and make the code less verbose.

(In reply to comment #4) > The code is incredibly repetitive. That's not the problem, the real problem is (as you said) manual modifications really are unexpected. > The attached patch is the start of a refactoring that would reduce > the file size by 100+ lines. Your patch covers only one of the dozen of encoding classes. > The code is automatically generated? I didn't know that. It's explicitly written in the source: /*************************************/ /* Start of the auto-generated part. */ ... /* End of the auto-generated part. */ /***********************************/ > If you have auto-generated code in a repository you should also > include the program that does the auto-generation. I can't do that, sorry. > In fact, the auto-generated code itself probably should not be in > the repository, because people will modify NativeSH4.cpp and that'll > get overwritten next time you auto-generate. This code was in a separated file but one asked me explicitly to put its content into NativeSH4.cpp: https://bugzilla.mozilla.org/show_bug.cgi?id=568486#c32 > You could still modify your auto-generation program to do the above > refactoring and make the code less verbose. I will work on that if you create a new Bugzilla entry for this.

> This code was in a separated file but one asked me explicitly to put > its content into NativeSH4.cpp: > > https://bugzilla.mozilla.org/show_bug.cgi?id=568486#c32 That was me. In that comment, I wrote: "If there is significant benefits to the extra file, then keep it." I didn't realize, and nobody mentioned, that it was an auto-generated file. Being generated is a VERY good reason to keep it separate! > > You could still modify your auto-generation program to do the above > > refactoring and make the code less verbose. > > I will work on that if you create a new Bugzilla entry for this. Would it be possible to check in the input to the code generator? I am guessing it is some kind of template since it must have many nanojit particulars in it.

(In reply to comment #6) > I didn't realize, and nobody mentioned, that it was an auto-generated file. It was explicitly mentioned at the beginning of this file. > Being generated is a VERY good reason to keep it separate! Hence my first patch. > I am guessing it is some kind of template since it must have many > nanojit particulars in it. Actually the code generator is NanoJIT specific, not its input.

Okay, cool (mea culpa, etc). Same question tho - is the "source" for the generator (the thing with nanojit particulars in it) so proprietary that it can't be checked in? I would understand if a propretary tool or machine model needed to be witheld, but I'm hoping we can find a useful balance that's more than just the generated one. regardless, I'm in favor of splitting the generated code into its own file like the first patch. (maybe something in keeping with the existing filenames, like: NativeSH4-generated.h, or -asm.h, etc).

So I'm not sure how we can move forward with the patch, as it seems that some generator logic (that we don't have access to) would require changes. Cédric how would you like to proceed? Would you mind to attempt the following 'cd utils/nanojit-import/ ; update-nanojit.sh tip ' using the latest tamarin-redux and then submit a patch for fixing the build? BTW, I think the script needs 'convert' enabled : http://mercurial.selenic.com/wiki/ConvertExtension#Configuration

(In reply to comment #5) > > > The attached patch is the start of a refactoring that would reduce > > the file size by 100+ lines. > > Your patch covers only one of the dozen of encoding classes. As I said, it's a start. If you need a dozen helpers, that's fine; it'll still end up being shorter and less repetitive. > > If you have auto-generated code in a repository you should also > > include the program that does the auto-generation. > > I can't do that, sorry. Why not? > > You could still modify your auto-generation program to do the above > > refactoring and make the code less verbose. > > I will work on that if you create a new Bugzilla entry for this. The SH4 backend is of little interest to me because it's not relevant to Mozilla. So the only thing I care about is that it cause as few problems to the other backends as possible. Rick has already wasted some time writing a patch for auto-generated code, which isn't good. And while there are some comments in the file explaining that it's auto-generated, Ed, Rick and I all missed them, so they're clearly not that obvious. So I agree with Ed that, at the very least, the auto-generated part should be separate and clearly named, eg. NativeSH4-auto-generated.cpp. As for the code quality within that file, I made a suggestion; whether you act on that is up to you.

(In reply to comment #9) > So I'm not sure how we can move forward with the patch, as it seems that some > generator logic (that we don't have access to) would require changes. > > Cédric how would you like to proceed? > > Would you mind to attempt the following 'cd utils/nanojit-import/ ; > update-nanojit.sh tip ' using the latest tamarin-redux and then submit a patch > for fixing the build? > > BTW, I think the script needs 'convert' enabled : > http://mercurial.selenic.com/wiki/ConvertExtension#Configuration OK, I will try to handle this problem next week. (In reply to comment #10) > The SH4 backend is of little interest to me because it's not > relevant to Mozilla. So the only thing I care about is that it > cause as few problems to the other backends as possible. Rick > has already wasted some time writing a patch for auto-generated > code, which isn't good. And while there are some comments in > the file explaining that it's auto-generated, Ed, Rick and I > all missed them, so they're clearly not that obvious. I'm deeply sorry if the SH4 backend caused problems to other backends (!?). Note you could add myself to the list of person who wasted time for something irrelevant. > So I agree with Ed that, at the very least, the auto-generated > part should be separate and clearly named, > eg. NativeSH4-auto-generated.cpp. As for the code quality > within that file, I made a suggestion; whether you act on that > is up to you. OK, I will move that part of the code in the file NativeSH4-auto-generated.cpp

landed assert fix directly in tamarin-redux in order to unblock merge, expect that those changes will be clobbered by auto-generated follow-up patch http://hg.mozilla.org/tamarin-redux/rev/ae17197d1a0d

I noticed patches for Bug 577449 and Bug 594296 are not yet committed, sadly this later requires same kind of changes. Do I have to resubmit a new patch for Bug 594296 then?

Comment on attachment 487531 [details] [diff] [review] SH4: use non-numeric type for Register & move auto-generated code-generator back to a separated file. R+ , please note attachment 486976 [details] [diff] [review] , where reginc() is being removed.

(In reply to comment #13) > Created attachment 487531 [details] [diff] [review] > I noticed patches for Bug 577449 and Bug 594296 are not yet > committed, sadly this later requires same kind of changes. True and I'm afraid this will continue to be the case, unless others have some form of access to the code generator (note: only input and output are needed) you're using. > Do I have to resubmit a new patch for Bug 594296 then? If it needs to be rebased then yes. Do you know why this didn't land? If you're not able to land it you can just ask one of the reviews to do so and then periodically looks for the whiteboard entry 'fixed-in-nanojit' and the checkin stamp in the comments to see that it does (assuming you're not signed up for bugzilla email).

(In reply to comment #14) > R+ , please note attachment 486976 [details] [diff] [review] , where reginc() is being removed. It looks like I didn't procrastinate enough. (In reply to comment #15) > (In reply to comment #13) > > > I noticed patches for Bug 577449 and Bug 594296 are not yet > > committed, sadly this later requires same kind of changes. > > True and I'm afraid this will continue to be the case, unless others have some > form of access to the code generator (note: only input and output are needed) > you're using. Actually Bug 577449 is not related to code generation at all; this is a generic bug regarding the C++ standard. For Bug 594296, I think access to the code-generator generator wouldn't have improved the delay between the end of review and the landing of the patch. Frankly, I'm ready to handle any changes related to the SH4 code generation since you can't have access to the code generator generator (no, I will not argue on that point ;)). > > Do I have to resubmit a new patch for Bug 594296 then? > > If it needs to be rebased then yes. OK, I will submit a new patch for Bug 594296 and another one for this entry (unless another change to class Register is on going). > Do you know why this didn't land? > > If you're not able to land it you can just ask one of the reviews to do so and > then periodically looks for the whiteboard entry 'fixed-in-nanojit' and the > checkin stamp in the comments to see that it does (assuming you're not signed > up for bugzilla email). I really don't know why these patches didn't land since they are R+. How can I push SH4 related patches by myself? In the meantime, please could you commit Bug 577449? Thanks in advance.

rreitmai http://hg.mozilla.org/tamarin-redux/rev/86a90b15f494

The path was updated to remove calls to REGINC(), this new patch requires attachment 472978 [details] [diff] [review].

remove "assert(Register >= 0)" + require attachment 473989 [details] [diff] [review]

We keep having to land attachment 485873 [details] [diff] [review], can we get a fix for it? Probably best to open a new bug for it.

Oops, attachment 489455 [details] [diff] [review] is not the right patch. My mistake, sorry.

(In reply to comment #20) > We keep having to land attachment 485873 [details] [diff] [review], can we get a fix for it? It was integrated in attachment 493930 [details] [diff] [review].

(In reply to comment #23) > (In reply to comment #20) > > We keep having to land attachment 485873 [details] [diff] [review], can we get a fix for it? > > It was integrated in attachment 493930 [details] [diff] [review]. Can you let me know how we can land this piece? It appears to be part of a series of patches. I.e build system changes and/or the file needs to be included somewhere. When all the changes are ready, feel free to mark the 2 patches (one by me , the other by nick as obsolete) and I'll review and land.

rreitmai http://hg.mozilla.org/projects/nanojit-central/rev/b681599a5426

cedric http://hg.mozilla.org/projects/nanojit-central/rev/eb3a73807a9c

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #20) > It appears to be part of a series of patches. Sorted it out ... attachment 493930 [details] [diff] [review] was dependent on the uncommitted patch 485873. Landed both

http://hg.mozilla.org/tracemonkey/rev/a19df7a3261f http://hg.mozilla.org/tracemonkey/rev/2b7d5628b519

cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/2b7d5628b519 http://hg.mozilla.org/mozilla-central/rev/a19df7a3261f