Closed Bug 1004116 Opened 11 years ago Closed 3 years ago

js/src/jit/MIR.h is too big

Categories

(Core :: JavaScript Engine: JIT, enhancement, P5)

Product:
Core
Component:
JavaScript Engine: JIT
Type:
enhancement

Tracking

()

Status:
RESOLVED INACTIVE

People

(Reporter: sunfish, Unassigned)

Details

Attachments

(10 obsolete files)

MIR.h is over 10000 lines long and is a good opportunity for refactoring. The main obvious thing to do here would be to split out all or some of the MDefinition subclasses into a separate header. Among other benefits, this should allow code which doesn't need all the subclass definitions to have ligher build dependencies. It may also make sense to look into whether there are any inline function bodies which make sense to be moved out of line.
I also approve these changes. I think some inline functions should move to the cpp files of the corresponding analysis / transformations phases. Among the thing that we can do: - Move the AliasSet logic (getAliasSet, isEffectful, mightAlias) to AliasAnalysis.cpp. - Move CongruentTo & foldsTo definitions to ValueNumbering.cpp - Use visitor instead of adding methods, as Hannes suggested for the Recover Instructions (in Bug 989667) - Do not inherit from TypePolicies, use a static pointer when the policies are state-less, or aggregate the type when it is not. - Add a default No-op type policy, and include use it for all operations in the INSTRUCTION_HEADER macro. - Make categories (control, math, object, string, array, fun, …) for the different kind of operations, and move the New & Constructors in MIR-<category>.cpp [this would be similar to jsfun.cpp, jsmath.cpp, …] - Make a simple Macro to desugar operand names: #define OperandNames3(N0, N1, N2) \ OperandNames2(N0, N1) \ MDefinition *N2() const { \ return getOperand(2); \ }
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > Among the thing that we can do: > - Move the AliasSet logic (getAliasSet, isEffectful, mightAlias) to > AliasAnalysis.cpp. > - Move CongruentTo & foldsTo definitions to ValueNumbering.cpp I strongly disagree with that. Having methods of a single class scattered among different files make it harder to find information, as you have to know in the first place where to look for. It happened to me a few times with computeRange() methods, that I would actually prefer to have in the MIR.{h,cpp} files too. At least, with the big MIR.{h,cpp}, I know all knowledge is in a single place and I don't have to think at "in which file is this supposed to be?". > - Do not inherit from TypePolicies, use a static pointer when the policies > are state-less, or aggregate the type when it is not. Agreed, most type policies classes are just wrappers to static functions. > - Add a default No-op type policy, and include use it for all operations in > the INSTRUCTION_HEADER macro. If we use a pointer, we can also just have a nullptr instead of a Noop type policy that doesn't do anything. > - Make categories (control, math, object, string, array, fun, …) for the > different kind of operations, and move the New & Constructors in > MIR-<category>.cpp [this would be similar to jsfun.cpp, jsmath.cpp, …] I strongly agree with this change. That sounds way better (to me, at least) than having methods scattered everywhere. Having some consistency with the js*.cpp files would be nice, but isn't a strong prerequisite.
(In reply to Benjamin Bouvier [:bbouvier] from comment #2) > (In reply to Nicolas B. Pierron [:nbp] from comment #1) > > Among the thing that we can do: > > - Move the AliasSet logic (getAliasSet, isEffectful, mightAlias) to > > AliasAnalysis.cpp. > > - Move CongruentTo & foldsTo definitions to ValueNumbering.cpp > > I strongly disagree with that. Having methods of a single class scattered > among different files make it harder to find information, as you have to > know in the first place where to look for. It happened to me a few times > with computeRange() methods, that I would actually prefer to have in the > MIR.{h,cpp} files too. All computeRange methods are in one file. The same and only file which is calling these methods. Moving such functions closer to their call sites simplify patches, by isolating modifications to one file, and also is likely to improve code-locality by collecting together functions which are logically linked. Also, having things which are logically linked close improves the maintainability of the code. Having all computeRange functions scattered in MIR.cpp would be a mistake, for example it would be easy to miss one of the function when the Range class semantic changes. > At least, with the big MIR.{h,cpp}, I know all > knowledge is in a single place and I don't have to think at "in which file > is this supposed to be?". I do not have to think, in which file it is supposed to be, because it is supposed to be in the same file, as long as this is a property/methods which is unique to this analysis/transformation. If you have to look at computeRange information, you better have to know about Range Analysis, especially knowing how easy it is to make mistakes.
May I try to work on this bug? I'm looking at the corresponding source now but I not too familiar with the structure of how things are organized. For example http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#15, I don't see mozilla directory in js/src/, how does it get included here? Thanks!
(In reply to Zuhao(Joe) Chen from comment #4) > May I try to work on this bug? I'm looking at the corresponding source now > but I not too familiar with the structure of how things are organized. For > example http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#15, I > don't see mozilla directory in js/src/, how does it get included here? > Thanks! Array.h is located in the mfbt folder, which is moved during compile time to the /mozilla/Array.h location, if I understand it correctly. You can view the source for Array.h at http://mxr.mozilla.org/mozilla-central/source/mfbt/Array.h?force=1.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > (In reply to Zuhao(Joe) Chen from comment #4) > > May I try to work on this bug? I'm looking at the corresponding source now > > but I not too familiar with the structure of how things are organized. For > > example http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#15, I > > don't see mozilla directory in js/src/, how does it get included here? > > Thanks! > > Array.h is located in the mfbt folder, which is moved during compile time to > the /mozilla/Array.h location, if I understand it correctly. You can view > the source for Array.h at > http://mxr.mozilla.org/mozilla-central/source/mfbt/Array.h?force=1. I see. Thanks! Do you know which script move these files and generate Makefile?
Flags: needinfo?(sunfish)
(In reply to Zuhao(Joe) Chen from comment #6) > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5) > > (In reply to Zuhao(Joe) Chen from comment #4) > > > May I try to work on this bug? I'm looking at the corresponding source now > > > but I not too familiar with the structure of how things are organized. For > > > example http://mxr.mozilla.org/mozilla-central/source/js/src/jit/MIR.h#15, I > > > don't see mozilla directory in js/src/, how does it get included here? > > > Thanks! > > > > Array.h is located in the mfbt folder, which is moved during compile time to > > the /mozilla/Array.h location, if I understand it correctly. You can view > > the source for Array.h at > > http://mxr.mozilla.org/mozilla-central/source/mfbt/Array.h?force=1. > > I see. Thanks! Do you know which script move these files and generate > Makefile? It happens during the regular build process, which is something you'll need to do anyway in order to test your changes. Instructions for building the standalone JS shell are here: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Flags: needinfo?(sunfish)
(In reply to Nicolas B. Pierron [:nbp] from comment #3) > (In reply to Benjamin Bouvier [:bbouvier] from comment #2) > > (In reply to Nicolas B. Pierron [:nbp] from comment #1) > > > Among the thing that we can do: > > > - Move the AliasSet logic (getAliasSet, isEffectful, mightAlias) to > > > AliasAnalysis.cpp. > > > - Move CongruentTo & foldsTo definitions to ValueNumbering.cpp > > > > I strongly disagree with that. Having methods of a single class scattered > > among different files make it harder to find information, as you have to > > know in the first place where to look for. It happened to me a few times > > with computeRange() methods, that I would actually prefer to have in the > > MIR.{h,cpp} files too. > > All computeRange methods are in one file. The same and only file which is > calling these methods. > Moving such functions closer to their call sites simplify patches, by > isolating modifications to one file, and also is likely to improve > code-locality by collecting together functions which are logically linked. > Also, having things which are logically linked close improves the > maintainability of the code. I can see merits to both approaches. Someone thinking about a particular class is better served by having all the methods for that class together in one place, but someone thinking about a particular feature, like ValueNumbering, is better served by having all the methods for that feature together in one place. I favor Nicolas' proposal for the present purpose. There are several refactorings that we're preparing to do to the interfaces of the ValueNumbering and AliasAnalysis methods, and they will be more convenient if the methods are grouped by functionality.
Mentor: sunfish
Whiteboard: [mentor=sunfish][lang=c++][good first bug] → [lang=c++][good first bug]
Assignee wants to work on the bug (and lacks 'editbugs' privileges).
Assignee: nobody → me
I think a good way to start would be to create a new header file and move all the subclasses of MInstruction into it. This would include utility subclasses like MUnaryInstruction and friends, and also the large number of classes that are subclasses of those. I propose naming this new file MIRInstructions.h.
(In reply to Dan Gohman [:sunfish] from comment #10) > I think a good way to start would be to create a new header file and move > all the subclasses of MInstruction into it. This would include utility > subclasses like MUnaryInstruction and friends, and also the large number of > classes that are subclasses of those. I propose naming this new file > MIRInstructions.h. Basically you mean, everything which is indexed in MOpcodes.h ?
Yes. I'd also include utility classes like MUnaryInstruction etc.
In order to avoid recursive including the declaration of MDefinition (MInstruction inherits from MDefinition which is also declared in MIR.h with all other ancestors of MDefinition) has to be removed from MIR.h too.
I think we can do it without moving MDefinition. In addition to moving the MInstructionn subclasses (and their subclasses) out, we'll need to move any code that depends on them out as well. That includes at least the OPCODE_CASTS code. MIR.h shouldn't retain anything that needs to depend on MIRInstructions.h, but MDefinition won't, so it can stay. BTW, I believe there is one MInstruction subclass which should stay in MIR.h, MControlInstruction, since it'd be nice to let MIRGraph.h include just MIR.h (and not MIRInstructions.h), and it uses toControlInstruction().
Well, we can move MInstruction (and all descendants) to MIRInstructions.h but almost every class within that file somehow inherits from MInstruction (well, every class inherits from M{Nullary,Unary,Binary,Ternary}Instruction). There is also a bunch of math related objects. I need to set up some clear rules which I can use to make decision on moving.
I propose leaving MInstruction in MIR.h and moving all its descendants (except MControlInstruction). This is indeed the great majority of classes. Even though this may seem like an uneven split, this split is a useful first step. Several important classes will remain in MIR.h, such as MNode, MDefinition, MResumePoint, MUse, and MInstruction. Code that doesn't need to think about specific instruction opcodes can #include just this new MIR.h and work with instructions through the MInstruction base class. Once this is done, we can look into other ways to reduce MIRInstructions.h, which as you say, will then contain lots of classes. Several inline functions should probably be outlined, and a variety of other ideas are discussed above too. FYI, in bug 1041673 I noticed that the way the to##opcode and is##opcode functions are defined will be a little awkward with this split, and a patch I had been working on for other reasons will actually fix it. I believe it's orthogonal, as the changes discussed in this bug should work with or without it, but it might make them a little cleaner.
Okay, I'm moving to MIRInstructions.h these classes: - MAryControlIntruction (and all subclasses) - M{Nullary,Unary,Binary,Ternary,Quaternary}Instruction (and all subclasses) - Classes that are before MInstruction (MInstruction included) will remain in MIR.h There are some helper functions and I am not not sure if I can move them so I'll leave them within MIR.h for now.
Sorry, I meant MAryInstruction. (In reply to me from comment #17) > Okay, I'm moving to MIRInstructions.h these classes: > - MAryControlIntruction (and all subclasses) > - M{Nullary,Unary,Binary,Ternary,Quaternary}Instruction (and all subclasses) > - Classes that are before MInstruction (MInstruction included) will remain > in MIR.h > > There are some helper functions and I am not not sure if I can move them so > I'll leave them within MIR.h for now.
Okay, I've done it, now MIR.h has 2200 lines and MIRInstructions.h 8700 lines, but I didn't compile it (I haven't resolved header-including issues yet)
It sounds like you're on the right track!
Okay guys, I've moved all things and resolved issue with macros (I'm not sure if I've done a right thing but at least it is able to compile), I have resolved issues in some files (headers): - jit/MIRInstructions.h includes jit/MIR.h - jit/IonAnalysis.h includes jit/MIRInstructions.h - jit/IonBuilder.h includes jit/MIRInstructions.h - jit/LIR.h includes jit/MIRInstructions.h Macro: OPCODE_CASTS moved into jit/MIRInstructions.h Placing "#undef INSTRUCTION_HEADER" at the end of jit/MIRInstructions.h MIR.h has less than 2k of lines. I'll make a patch ASAP.
Well, everything seems pretty fine but my MQ patch file is 500kB big (Well, MIR.h is big too) as I've done it in one "session" it won't be so nicely reviewable (and I must admit I am not familiar with mercurial - I am mainly git user). So what shall I do now?
500kB for the resulting diff sounds about what I'd expect. While splitting up large patches into small changes is often very useful, for this kind of change it's less important. I don't mind a monolithic patch for this change. The next step is to attach the patch to this bug and mark it r? :sunfish to request a review. I may ask for changes, or I may approve it as is. Once approved, I can land it for you.
The first attempt.
Attachment #8462722 - Flags: review?(sunfish)
Comment on attachment 8462722 [details] [diff] [review] Patch fixing bug 1004116 (bug-1004116-fix.patch) Review of attachment 8462722 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks close to what we want. I do have some changes to request, though I believe most of it should be easy to incorporate, so don't get discouraged! Let me know if anything is unclear. I apologize for not thinking of this before, but one thing you can do to reduce the size of the patch and make it easier for me to review is to create MIRInstructions.h with "hg cp". This will tell Mercurial how to track MIRInstructions.h's history. You can do this without starting over by moving your MIRInstructions.h and MIR.h out of the way, then doing "hg cp MIR.h MIRInstructions.h", and then moving your MIR.h and MIRInstructions.h back in place. Then after a hg qref, the patch file should be smaller and should more clearly show what changes were made in the split. The patch doesn't compile for me; BaselineInspector.h uses MCompare::CompareType, so it requires MIRInstructions.h. EffectiveAddressAnalysis.cpp uses MLsh and other things, so it requires MIRInstructions.h. Possibly there are others but I didn't look further. Were you able to complete a build with this patch? What kind of build did you do? With this patch, MGoto, MTest, MReturn, and others are still in MIR.h. I apologize for being unclear; I'd like these instructions to move into MIRInstructions.h with the other instruction classes. I meant that the MControlInstruction base class itself should stay in MIR.h, but its subclasses can all move. Please include a commit message in the patch. See "hg log" for some examples of the style. With MQ, you can edit the commit message by running "hg qref -e". I find it convenient to have the following lines in my .hg/hgrc: [defaults] qnew = -Ue With this, when I run hg qnew, it automatically pops up an editor where I can enter a commit message. ::: js/src/jit/MIR.h @@ -5683,1 @@ > struct LambdaFunctionInfo Ah, I forgot that there were several utility classes like this. Looking around, I think LambdaFunctionInfo, MStoreElementCommon, InlinePropertyTable, CacheLocationList, MRestCommon, MAsmJSHeapAccess are all pretty closely tied to specific instruction subclasses, so they should go in MIRInstructions.h with the instructions they accompany. ::: js/src/jit/MIRInstructions.h @@ +196,5 @@ > + MUse *getUseFor(size_t index) MOZ_FINAL MOZ_OVERRIDE { > + return &operands_[index]; > + } > + const MUse *getUseFor(size_t index) const MOZ_FINAL MOZ_OVERRIDE { > + return &operands_[index]; This line has a spurious trailing whitespace. @@ +6832,5 @@ > + > + > + > + > + Watch out for excessive newlines. @@ +8443,5 @@ > + return AliasSet::None(); > + } > +}; > + > +// This is an alias for MLoadFixedSlot. (MUnaryInstruction) I'm curious why you added "(MUnaryInstruction)" to this comment. @@ +8641,5 @@ > + return AliasSet::None(); > + } > +}; > + > +class MIsCallable It looks like some things got reordered; MIsCallable went from being followed by MIsObject to being followed by MAsmJSNeg. The order isn't particularly important, but preserving the existing order would make it easier for me to find the interesting changes. @@ +8992,5 @@ > + return JSOpToCondition(op, isSigned); > +} > + > + > +} // namespace jit This line has a spurious trailing whitespace.
Attachment #8462722 - Flags: review?(sunfish)
I see, I'll fix it. That note I've made was just a notice from what inherited MLoadFixedSlot.
(In reply to Dan Gohman [:sunfish] from comment #25) > Comment on attachment 8462722 [details] [diff] [review] > Patch fixing bug 1004116 (bug-1004116-fix.patch) > > Review of attachment 8462722 [details] [diff] [review]: > ----------------------------------------------------------------- > > I apologize for not thinking of this before, but one thing you can do to > reduce the size of the patch and make it easier for me to review is to > create MIRInstructions.h with "hg cp". This will tell Mercurial how to track > MIRInstructions.h's history. You can do this without starting over by moving > your MIRInstructions.h and MIR.h out of the way, then doing "hg cp MIR.h > MIRInstructions.h", and then moving your MIR.h and MIRInstructions.h back in > place. Then after a hg qref, the patch file should be smaller and should > more clearly show what changes were made in the split. Do I have to do it now and again? > The patch doesn't compile for me; BaselineInspector.h uses > MCompare::CompareType, so it requires MIRInstructions.h. > EffectiveAddressAnalysis.cpp uses MLsh and other things, so it requires > MIRInstructions.h. Possibly there are others but I didn't look further. Were > you able to complete a build with this patch? What kind of build did you do? I've built only SpiderMonkey not the whole Firefox. > With this patch, MGoto, MTest, MReturn, and others are still in MIR.h. I > apologize for being unclear; I'd like these instructions to move into > MIRInstructions.h with the other instruction classes. I meant that the > MControlInstruction base class itself should stay in MIR.h, but its > subclasses can all move. > > Please include a commit message in the patch. See "hg log" for some examples > of the style. With MQ, you can edit the commit message by running "hg qref > -e". > > I find it convenient to have the following lines in my .hg/hgrc: > > [defaults] > qnew = -Ue > > With this, when I run hg qnew, it automatically pops up an editor where I > can enter a commit message. Alright, sure.
I have no problem with compiling whatsoever even full clean build compiles. Don't you have applied any other patches that might be causing problems? But I have to admit, I haven't update the local repository yet.
When I updated repository and I tried to build firefox it's not able to compile (some issues with widgets/cocoa/nsChildView.mm)... now it's broken (at least for me).
(In reply to me from comment #27) > (In reply to Dan Gohman [:sunfish] from comment #25) > > Comment on attachment 8462722 [details] [diff] [review] > > Patch fixing bug 1004116 (bug-1004116-fix.patch) > > > > Review of attachment 8462722 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I apologize for not thinking of this before, but one thing you can do to > > reduce the size of the patch and make it easier for me to review is to > > create MIRInstructions.h with "hg cp". This will tell Mercurial how to track > > MIRInstructions.h's history. You can do this without starting over by moving > > your MIRInstructions.h and MIR.h out of the way, then doing "hg cp MIR.h > > MIRInstructions.h", and then moving your MIR.h and MIRInstructions.h back in > > place. Then after a hg qref, the patch file should be smaller and should > > more clearly show what changes were made in the split. > > Do I have to do it now and again? No, once is enough. > > > The patch doesn't compile for me; BaselineInspector.h uses > > MCompare::CompareType, so it requires MIRInstructions.h. > > EffectiveAddressAnalysis.cpp uses MLsh and other things, so it requires > > MIRInstructions.h. Possibly there are others but I didn't look further. Were > > you able to complete a build with this patch? What kind of build did you do? > > I've built only SpiderMonkey not the whole Firefox. Hmm, SpiderMonkey ought to be enough here. That's what I was building when I saw the errors. (In reply to me from comment #28) > I have no problem with compiling whatsoever even full clean build compiles. > Don't you have applied any other patches that might be causing problems? But > I have to admit, I haven't update the local repository yet. I tested it in a tree with no other changes. I updated to the parent revision in the patch. Is it possible that you made changes in your tree without refreshing the patch (hg qrefresh)? (In reply to me from comment #29) > When I updated repository and I tried to build firefox it's not able to > compile (some issues with widgets/cocoa/nsChildView.mm)... now it's broken > (at least for me). I don't know what that's about, but it's surely unrelated. I'm seeing that EffectiveAddressAnalysis.cpp and a few others need definitions from MIRInstructions.h and aren't getting them, so it looks like it's just a case of a few missing #includes.
Updated first patch. Fixing headers, etc.
Attachment #8463095 - Flags: review?(sunfish)
(Note: I am using OS X 10.10 Yosemite.) That EffectiveAddressAnalysis.cpp doesn't include MIRInstructions.h directly (and I'm not gonna spend my time in this graph/dependency issue resolving by hand) but still clang compiles without any issues. That's strange, I've started wondering why it does work and compile.
Oh, I just realized what it is: Unified builds. To speed up build times, the build system concatenates many C++ source files into large combined C++ source files. On rare occasions someone forgets a #include and gets lucky, and things still work because that file gets concatenated with other files that do have the #include. So, for working on this bug, you should disable unified builds, so that we don't end up depending on getting lucky. You can do this by configuring with --disable-unified-compilation.
Recompiled with --disable-unified-compilation and fixed missing includes.
Attachment #8463113 - Flags: review?(sunfish)
Attachment #8462722 - Attachment is obsolete: true
Attachment #8463095 - Attachment is obsolete: true
Attachment #8463095 - Flags: review?(sunfish)
Comment on attachment 8463113 [details] [diff] [review] Fixing includes within multiple files. (bug-1004116-fix.patch) Review of attachment 8463113 [details] [diff] [review]: ----------------------------------------------------------------- Alright, everything builds for me, the "hg cp" looks good, and overall the split looks good. There are just a few more details here. ::: js/src/jit/MIR.h @@ +1404,5 @@ > return static_cast<const M##opcode *>(this); \ > } > MIR_OPCODE_LIST(OPCODE_CASTS) > #undef OPCODE_CASTS > +*/ We generally try to avoid having commented-out code sitting around in the tree. These cast functions are now defined in MIRInstructions.h, so we can just delete this copy, rather than just commenting it out. ::: js/src/jit/MIRInstructions.h @@ +28,5 @@ > } \ > bool accept(MInstructionVisitor *visitor) { \ > return visitor->visit##opcode(this); \ > } > +*/ Since this code is staying in MIR.h, we should removed this commented-out copy. @@ -1241,5 @@ > - > - void printOpcode(FILE *fp) const; > -}; > - > -class MTableSwitch MOZ_FINAL MTableSwitch should move to MIRInstructions.h with the other subclasses. @@ -1382,5 @@ > - } > -}; > - > -template <size_t Arity, size_t Successors> > -class MAryControlInstruction : public MControlInstruction MAryControlInstruction can move to MIRInstructions.h too.
Attachment #8463113 - Flags: review?(sunfish)
Attachment #8463113 - Attachment is obsolete: true
Attachment #8466109 - Flags: review?(sunfish)
Comment on attachment 8466109 [details] [diff] [review] Removed comment, moved MTableSwitch and MAryControlInstruction (bug-1004116-fix.patch) Review of attachment 8466109 [details] [diff] [review]: ----------------------------------------------------------------- Cool, this looks good. This is a good first step towards making this code easier to work with.
Attachment #8466109 - Flags: review?(sunfish) → review+
Alright, the email notified me and asked me to push a change into a repo, I'm not sure if I should do it right now.
Ok, I apologize for this, but it's actually inconvenient for us to land this patch right now; we're right in the middle of landing a big series of patches for SIMD support (children of bug 894105), and I'm afraid it seems better to ask you to wait for the moment and then rebase your patch, than to juggle the rebasing on the SIMD patch series. I do appreciate your working on this, and I hope it doesn't have to wait long.
That's okay, I'll read some documentation at MDN till the time come. I'll watch that bug and let me know when it's possible to do it.
FYI, the (first) SIMD patch series landed today (last mozilla-inbound revision: 043edc92814e). My apologies for blocking your work which looks nice, btw :)
(In reply to Benjamin Bouvier [:bbouvier] from comment #41) > FYI, the (first) SIMD patch series landed today (last mozilla-inbound > revision: 043edc92814e). My apologies for blocking your work which looks > nice, btw :) C'mon, it's just refactoring :D and in my opinion still not done...
can i seek for ur help in getting this problem sorted....... can u assign this work up for me
(In reply to kiransweetheart26 from comment #43) > can i seek for ur help in getting this problem sorted....... can u assign > this work up for me Sorry but you are too late. It's fixed, but not yet pushed and closed.
Dan, what's the status here?
(In reply to Josh Matthews [:jdm] from comment #45) > Dan, what's the status here?
Flags: needinfo?(sunfish)
Well, I'm just sitting here and waiting for some notification :) (however I had a lot of work to do so I didn't keep the track of changes that have been made in time.) but I didn't give it up :)
SIMD has settled down for now, as far as MIR.h goes, especially after bug 1113338 lands. And, the PJS removal is now done. I'm not aware of any other major upcoming MIR.h churn, so we may be arriving at a good time to start up this bug again here. Does anyone else know of any upcoming conflicts? Thanks again for your patience here!
Flags: needinfo?(sunfish)
It would be great if this could be done during the 38 cycle to avoid future patch backport pain on ESR38.
I'm not aware of any major upcoming changes right now, so now would be a good time to revive this patch, if you're still interested :).
I think this may need a rebase :D
Priority: -- → P5
This is my first attempt at contributing to the Mozilla code base. I am a university student that would appreciate any advice. Thanks!
Attachment #8928025 - Flags: review+
Attachment #8928025 - Flags: feedback+
Comment on attachment 8928025 [details] [diff] [review] Work in progress. Moved MDefinition subclasses into MIRSub.h Review of attachment 8928025 [details] [diff] [review]: ----------------------------------------------------------------- Hello! This patch doesn't contain the new MIRSub.h file; please be sure to do "hg add" when adding new files so that they're included in the patch. Also, much of the contents of MIR.h is subclasses of MInstruction, so moving MInstruction out to MIRSub.h would effectively make MIR.h depend on MIRSub.h. I recommend starting by moving out some of the leaves of the class hierarchy, as those are the things which don't have subsequent dependencies in MIR.h, and which fewer other source files depend on. Thanks!
Attachment #8928025 - Flags: review+
Attachment #8928025 - Flags: feedback+
Instead of MIRSub.h I would prefer MIRInstructions.h, MIRIns.h, or something.
Hi Dan, thanks for the comment. Apologies for not including MIRSub.h, I'm in the process of acquiring commit status so I won't be able to submit a patch (using hg add) until then. In the meantime, I'll investigate the class hierarchy and place the MInstruction subclasses back into MIR.h. Hi Jan, what exactly is wrong with MIRSub? So that I can adopt a better naming convention for future file additions. P.S. If either of you could vouch for me so that I can obtain commit status. I'd greatly appreciate it. The link to my bug report is: https://bugzilla.mozilla.org/show_bug.cgi?id=1417882
I've made some progress with identifying all of the leaf classes within the MDefintiion class hierarchy. They have been moved to a new file named 'MIRIns.h'. Would the next step be to identify and change all the files that depend on the leaf classes? By changing #include "MIR.h" to #include "MIRIns.h".
Attached file 1004116 (obsolete) —
Moves leaf classes of MDefinition hierarchy to a separate 'MIRIns.h' file.
Attachment #8928025 - Attachment is obsolete: true
Attachment #8929104 - Flags: review+
Attachment #8929104 - Flags: feedback+
Attached file MIRIns.h (obsolete) —
You can use "hg add" without having commit access, and that will allow you to create a single patch that has all of your changes in it. That will make it easier to review :-). If you wish, you can also use MozReview, without having commit access. See this page: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html and the links at the bottom for setting up specific version control tools. You should use the HTTPS access method for now. We don't typically vouch or grant commit access until after you've had some patches landed and are familiar with the process. Bugs like this one an be a first step though!
I've been using the Mozilla build VM which uses SSH to communicate with MozReview (LDAP account required). Is cloning and pushing from my host machine a workaround for this?
You should be able to change that by replacing the ssh:// in your ~/.hgrc file with https:// instead.
For a change like this, it's best if you can combine all the patches together into a single patch, so that reviewers can see exactly what's changed all at once. This page explains some options for how to do this in Mercurial: https://www.mercurial-scm.org/wiki/ConcatenatingChangesets
Attachment #8930021 - Attachment is obsolete: true
Apologies Dan, I've been trying to get a hang of using mercurial on mozilla-central, so there are changesets that should be ignored as they don't correctly reflect the changes that I've made. The changeset that I'd like reviewing is change set: 392663:fa542beb6635, commit id: Di5aT8AM25a. I've moved all MNullary, unary, binary, ternary and quaternary instruction subclasses into MInstruction.h. It has led to the reduction in LOC of MIR.h by approx. 5,000 lines. MIRInstruction.h's size is significant, would you recommend separating each MInstruction type into its own file? All contained within a new MIRInstruction directory? Thanks
Attachment #8930016 - Attachment is obsolete: true
Attachment #8929104 - Attachment is obsolete: true
Attachment #8929104 - Attachment is patch: false
Attachment #8929106 - Attachment is obsolete: true
Attachment #8466109 - Attachment is obsolete: true
Comment on attachment 8930017 [details] Bug 1004116 - Moved 'MNullaryInstruction', 'MUnaryInstrction', 'MBinaryInstruction', 'MTernaryInstruction' and 'MQuaternaryInstruction' subclasses into 'MIRInstruction.h'. Dependencies of these children by other files requires investigation. https://reviewboard.mozilla.org/r/201222/#review209548 This looks like a good start! I think it's ok for now that MIRInstruction.h is still quite large. We can take one step at a time. It looks like the next thing to do here is figure out which files need to include "MIRInstruction.h". In theory, there should be files that can juse use "MIR.h" and don't need "MIRInstruction.h". ::: commit-message-7acd8:1 (Diff revision 2) > +Bug 1004116 - Moved 'MNullaryInstruction', 'MUnaryInstrction', 'MBinaryInstruction', 'MTernaryInstruction' and 'MQuaternaryInstruction' subclasses into 'MIRInstruction.h'. Dependencies of these children by other files requires investigation. In a commit message, the first line should be a relatively short summary. You can add extra comments on subsequent lines.
Attachment #8930017 - Attachment is obsolete: true
Hello, This is bug still open or it has been fixed? If it is open, I'm new to the community and will like to take it as my first bug.
Attachment #8930016 - Attachment is obsolete: true
It's still open, and there hasn't been any activity for a while, so I think it's ok to take it. One thing to keep in mind are that while this change is conceptually simple (provided you're familiar with C++ header file mechanics), it'll be a big textual change, in a file where there are occasionally other large changes happening. I think moving all the subclasses of MInstruction (except MControlInstruction) and the things they depend on out of MIR.h into MIRInstructions.h is still a good first step here.
I'm no longer available to mentor this bug at this time.
Mentor: sunfish
Based on the amount of code to move around and that this bug lasted 5 years. I will remove the good-first-bug tag as well.
Whiteboard: [lang=c++][good first bug]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: me → nobody
Flags: needinfo?(sdetar)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(sdetar)
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: