Closed Bug 1044022 Opened 5 years ago Closed 5 years ago

Clean-up: IonMonkey: the LIRGenerator should inherit from MDefinitionVisitor instead of MDefinitionVisitorDefaultNYI

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: nbp, Assigned: inanc.seylan, Mentored)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 1 obsolete file)

Currently the LIR Generator classes are mostly implementing all instructions, except for a few exceptions such as Recover-only Instructions.  As we implement most of the MIR instructions, it would help new comers to identify which class they have to modify when they add a new MIR Instruction.

The goal of this bug is to change the inheritance such as our LIR generators enforce to implement the corresponding visitFoo functions.  If some instructions are missing we should manual add a MOZ_ASSERT_UNREACHABLE, which give a correct reason, such as "Unexpected Lowering of Recover Instruction" or "NYI".

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/shared/Lowering-shared.h#26
Whiteboard: [good first bug][lang=c++]
Just to understand the nature of the bug: you want that LIRGeneratorShared inherits from MInstructionVisitor instead of MInstructionVisitorWithDefaults. Since MInstructionVisitor has pure virtual methods, one then has to implement 217 (I think) visit methods in LIRGeneratorShared. LIRGeneratorShared uses only one of those methods: visitConstant (as far as I can tell). So about the remaining 216 visit method implementations that need to give the correct reason with MOZ_ASSERT_UNREACHABLE: would you use a macro or write them one by one in the relevant header and source files?
(In reply to Inanc Seylan from comment #1)
> Just to understand the nature of the bug: you want that LIRGeneratorShared
> inherits from MInstructionVisitor instead of
> MInstructionVisitorWithDefaults. Since MInstructionVisitor has pure virtual
> methods, one then has to implement 217 (I think) visit methods in
> LIRGeneratorShared. LIRGeneratorShared uses only one of those methods:
> visitConstant (as far as I can tell). So about the remaining 216 visit
> method implementations that need to give the correct reason with
> MOZ_ASSERT_UNREACHABLE: would you use a macro or write them one by one in
> the relevant header and source files?

No the idea is not to write them in the LIRGeneratorShared but write the missing one by hand in the LIRGenerator [1].

http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Lowering.h#31
Ok, I will give this bug a go.
Attached patch bug1044022.patch (obsolete) — Splinter Review
A first patch.
Attachment #8467094 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8467094 [details] [diff] [review]
bug1044022.patch

Review of attachment 8467094 [details] [diff] [review]:
-----------------------------------------------------------------

This is good, do the same thing for ArrayState, after rebasing on top of mozilla-inbound.

::: js/src/jit/Lowering.cpp
@@ +3814,5 @@
> +bool
> +LIRGenerator::visitPhi(MPhi *phi)
> +{
> +    // Phi nodes are not lowered because they are only meaningful for the register allocator.
> +    MOZ_ASSUME_UNREACHABLE("Unexpected Phi node during Lowering."); 

nit: Remove trailing whitespaces.

@@ +3822,5 @@
> +LIRGenerator::visitBeta(MBeta *beta)
> +{
> +    // Beta nodes are supposed to be removed before because they are
> +    // only used to carry the range information for Range analysis
> +    MOZ_ASSUME_UNREACHABLE("Unexpected Beta node during Lowering.");  

nit: same here.

@@ +3829,5 @@
> +bool
> +LIRGenerator::visitObjectState(MObjectState *objState)
> +{
> +    // ObjectState nodes are always recovered on bailouts
> +    MOZ_ASSUME_UNREACHABLE("Unexpected ObjectState node during Lowering.");  

nit: and here.
Attachment #8467094 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8467094 - Attachment is obsolete: true
Attached patch bug1044022.patchSplinter Review
Ok, here is the new patch incorporating:
1. rebased on top of mozilla-inbound;
2. trailing whitespaces removed;
3. ArrayState is taken care of.
Attachment #8467780 - Flags: review?(nicolas.b.pierron)
Assignee: nobody → inanc.seylan
Comment on attachment 8467780 [details] [diff] [review]
bug1044022.patch

Review of attachment 8467780 [details] [diff] [review]:
-----------------------------------------------------------------

Great :)
Do you recall how to use the try server?
Attachment #8467780 - Flags: review?(nicolas.b.pierron) → review+
Ok it looks like the tests have passed:

https://tbpl.mozilla.org/?tree=Try&rev=ce983bd1bab9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4db3058a5c9f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.