869313 - PJS: Refactor par MIRs and LIRs

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

We have a bunch of {M,L}Par* that really only differ from their sequential versions in that they take a ForkJoinSlice *. A refactor would be nice.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: WIP

A first draft at refactoring. Niko, do you think this approach is even cleaner? Converted MParLambda as an example.

Comment 2

[Niko Matsakis [:nmatsakis]]

Comment on attachment 746236 [details] [diff] [review]
WIP

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

This looks really good.

Comment 3

[Shu-yu Guo [:shu]]

Attachment: patch

Refactor of several Par MIR/LIR to use a variety of MAryInstruction that's optionaly +1 arity for when it's parallelized.

Dave, if you don't want to review this and think it's fine for nmatsakis to review it, feel free to flip the review to him.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: v2

Both sstangl and nbp expressed concern about the dynamic numOperands() in the previous version. This version keeps arity fixed for both MIR and LIR, and fill the unused ForkJoinSlice during seq. execution with an MConstant::New(UndefinedValue()), which gets lowered into a constant value LAllocation, so should exert no regalloc pressure.

Comment 5

[Shu-yu Guo [:shu]]

Attachment: v3

Change LParallelizableAllocInstructionHelper to have Min(2, Temps) number of temps instead of always Temps + 2, as the 2 temps needed for parallel alloc should always be able to be reused after allocation is done.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 747806 [details] [diff] [review]
v3

Talked with dvander about this on IRC; he feels separate IR is cleaner but said to defer to your opinion, so flipping r? to you.

I still feel this refactor makes things a bit cleaner in not duplicating MIR/LIR, especially for IR with more complicated codegen than what's refactored here (allocation).

Like I said during the talk at the work week, I'd like the plan going forward to not have duplicate IR for parallel/sequential modes. If you disagree, please advise.

Comment 7

[Shu-yu Guo [:shu]]

Some followup: there are already other IR that operate in both modes.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 747806 [details] [diff] [review]
v3

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

It's a tough call but I think I agree with dvander. Looking at NewObject, codegen for sequential/parallel is different anyway and MParallelizableAryInstruction, lowerParallelizable etc add (more) complexity for people not too familiar with Ion or parallel execution.

With extra MIR instructions there's some duplication and extra boilerplate, but it's code most of us can ignore most of the time (like the AsmJS* instructions) and it feels less complicated.

Comment 9

[Shu-yu Guo [:shu]]

Closing since refactoring isn't wanted.