1627618 - Remove no longer used binary arithmetic/binary instructions with no specialisations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P1)

Description

[André Bargull [:anba]]

After bug 1626297, MBinaryArithInstruction, MBinaryBitwiseInstruction, and MBitNot are always specialised to number types (MIRType::{Int32, Int64, Double, Float32}), so we can remove all code paths down to codegen which handle the case when there's no specialisation.

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 1: Add MAdd with truncate parameter. r=jandem!

Later patches will require a constructor with the signature
MAdd(MDefinition, MDefinition, MIRType), which neither sets the truncation
mode nor sets the "commutative" flag. Both things happen with the existing
constructor which has a compatible signature and which is currently called
for the use sites modified in this patch. (The existing constructor is the
one directly appearing before the newly added one.)

The existing constructor defaults to MDefinition::Truncate and all callers to
that constructor use the MIRType::Int32 specialisation. Because all callers
use MIRType::Int32, we can omit the type parameter for the new constructor.

The existing constructor is also called from WasmIonCompile, the next part will
handle that caller.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 2: Add MAdd::NewWasm for MAdd users in wasm code. r=jandem!

Add MAdd::NewWasm similar to how we have a separate MMul:NewWasm function.

For wasm we want to default to MDefinition::Truncate and set the "commutative"
flag for Int32 values.

This frees up the MAdd(MDefinition, MDefinition, MIRType) to only set the
specialisation and the return type, which will be required for later patches to
work correctly.

Depends on D69778

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 3: Add MSub::NewWasm for MSub users in wasm code. r=jandem!

Similar to the previous part, add MSub::NewWasm to handle the wasm-specific
initialisation bits, so that MSub(MDefinition, MDefinition, MIRType) only
sets the return type and the specialisation.

Depends on D69779

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 4: Replace calls to setInt32Specialization for MAdd. r=jandem!

The new MAdd and MSub constructors with a MIRType argument can be used
in these places to directly set the return type and the specialisation. That
way we no longer have to call setInt32Specialization() manually.

MMul already has a compatible constructor which the desired semantics, so
we can remove the setInt32Specialization() call here, too.

Depends on D69780

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 5: Directly set specialisation when calling MBinaryArithInstruction::New. r=jandem!

Instead of manually adjusting the specialisation in IonBuilder::binaryArithEmitSpecialized(),
directly pass it to MBinaryArithInstruction::New().

The constructor changes in the previous parts made it possible to pass MIRType
to the MAdd and MSub constructors.

Depends on D69781

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 6: Remove no longer used MBinaryArithInstruction sub-class constructors without a specialisation type. r=jandem!

MAdd, MSub, MMul, MDiv, and MMod are no longer called without a
number specialisation, so we can remove these constructors.

Depends on D69783

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 7: Set MBinaryArithInstruction specialisation in the constructor. r=jandem!

Set the specialisation and the return type directly in MBinaryArithInstruction.

Also assert that the specialisation is definitely a number type.

Depends on D69785

Comment 8

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 8: Remove MBinaryArithInstruction which handles non-number specialisations. r=jandem!

The last part ensures that MBinaryArithInstruction always uses a number
specialisation, so we can now remove any code which handles non-number
specialisations for MBinaryArithInstruction.

Depends on D69787

Comment 9

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 9: Use setSpecialization when setting MBinaryArithInstruction specialisation. r=jandem!

Update callers which modify the MBinaryArithInstruction specialisation to use
setSpecialization(), which asserts the new specialisation is definitely a
number type.

Depends on D69788

Comment 10

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 10: Remove no longer used codegen for non-number binary arith instructions. r=jandem!

MBinaryArithInstruction no longer uses non-number specialisations, so we can
remove the codegen support for it.

Depends on D69789

Comment 11

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 11: Remove constant argument from IonBuilder::binaryBitOpEmit. r=jandem!

IonBuilder::binaryBitOpEmit() is always called with specialization == Int32,
so we can remove the specialisation parameter and instead directly handle it
within binaryBitOpEmit().

Depends on D69790

Comment 12

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 12: Inline binaryBitOpEmit into its single caller. r=jandem!

Inline binaryBitOpEmit() into binaryBitOpTrySpecialized(). This makes it
easier to see that both operands are "simple bit-operands" when applying this
optimisation.

This change is useful for the next part, which will remove
MBinaryBitwiseInstruction::infer(), because infer() also tests that the
operands are "simple bit-operands".

Depends on D69791

Comment 13

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 13: Remove MBinaryBitwiseInstruction::infer(). r=jandem!

The different infer() overrides were all testing that the operands are
"simple bit-operands", but as demonstrated in the last part, we were no longer
calling infer() with non-simple bit-operands.

Instead let's effectively inline infer() and set the appropriate flags for
the individual MIR nodes:

• BitAnd, BitOr, BitXor: MBinaryBitwiseInstruction::specializeAs() isn't a
  public method, so we have to call setInt32Specialization() and
  setCommutative() separately. (At least for now, later patches will clean
  this up.)
• Lsh and Rsh: Call setInt32Specialization() to set the return type and the
  specialisation, similar to what happened in MShiftInstruction::infer().
• Ursh: Add MUrsh::setDoubleSpecialization() as a temporary mean to set the
  Double specialisation. Otherwise handle it similar to Lsh and Rsh.

Depends on D69792

Comment 14

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 14: Add MUrsh::NewWasm for MUrsh users in wasm code. r=jandem!

Similar to MAdd::NewWasm and MSub::NewWasm from earlier patches,
add a separate MUrsh::NewWasm function to handle wasm-specific
initialisation for MUrsh.

That frees up the MUrsh(MDefinition, MDefinition, MIRType) constructor to
only set the specialisation without performing any wasm-specific actions.

Depends on D69793

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 15: Set specialisation within MBinaryBitwiseInstruction constructor calls. r=jandem!

Similar to the earlier MBinaryArithInstruction changes, directly set the number
specialisation when constructing a MBinaryBitwiseInstruction node.

That way we no longer have to call setInt32Specialization() and
setCommutative() manually after constructing a binary bitwise instruction.

Depends on D69794

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 16: Remove MBinaryBitwiseInstruction::specializeAs. r=jandem!

Remove MBinaryBitwiseInstruction::specializeAs() and instead directly set the
specialisation in the MBinaryBitwiseInstruction constructor.

For BitAnd, BitOr, and BitXor also set the "commutative" flag in the constructor.

Depends on D69795

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 17: Remove MBinaryBitwiseInstruction which handles non-number specialisations. r=jandem!

'MBinaryBitwiseInstruction` is now only used for number specialisations, so we
can remove any non-number specialisation code.

Depends on D69796

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 18: Remove no longer used codegen for non-number binary bitwse instructions. r=jandem!

Removes the codegen support for non-number binary bitwse instructions.

The if-statements conditions in Lowering.cpp were changed to test ins->type()
instead of lhs->type() to match the binary arithmetic lowering code.

Depends on D69797

Comment 19

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 19: Don't call the always-inline UrshOperation in some places. r=jandem!

Noticed while working on the previous part:
js::UrshOperation() is marked as MOZ_ALWAYS_INLINE, but these callers don't
need to use the inlined version and instead should use js::UrshValues().

Depends on D69798

Comment 20

[André Bargull [:anba]]

Attachment: Bug 1627618 - Part 20: Change MBitNot to always specialise to Int32. r=jandem!

Same deal as with the previous parts, changes MBitNot to always specialise as
Int32 and removes any code which handled non-Int32 specialisation.

Depends on D69799

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43db59ebd874
Part 1: Add MAdd with truncate parameter. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b801d8007636
Part 2: Add MAdd::NewWasm for MAdd users in wasm code. r=jandem
https://hg.mozilla.org/integration/autoland/rev/bd3e77b5b2e9
Part 3: Add MSub::NewWasm for MSub users in wasm code. r=jandem
https://hg.mozilla.org/integration/autoland/rev/2ba73b4b8fd7
Part 4: Replace calls to setInt32Specialization for MAdd. r=jandem
https://hg.mozilla.org/integration/autoland/rev/fba058bb2204
Part 5: Directly set specialisation when calling MBinaryArithInstruction::New. r=jandem
https://hg.mozilla.org/integration/autoland/rev/45396902c724
Part 6: Remove no longer used MBinaryArithInstruction sub-class constructors without a specialisation type. r=jandem
https://hg.mozilla.org/integration/autoland/rev/f37a78d23533
Part 7: Set MBinaryArithInstruction specialisation in the constructor. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e14f372e1c71
Part 8: Remove MBinaryArithInstruction which handles non-number specialisations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/13791501ae5d
Part 9: Use setSpecialization when setting MBinaryArithInstruction specialisation. r=jandem
https://hg.mozilla.org/integration/autoland/rev/d4fa8213b437
Part 10: Remove no longer used codegen for non-number binary arith instructions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5b587fcde948
Part 11: Remove constant argument from IonBuilder::binaryBitOpEmit. r=jandem
https://hg.mozilla.org/integration/autoland/rev/40a10f393143
Part 12: Inline binaryBitOpEmit into its single caller. r=jandem
https://hg.mozilla.org/integration/autoland/rev/4802a1f03448
Part 13: Remove MBinaryBitwiseInstruction::infer(). r=jandem
https://hg.mozilla.org/integration/autoland/rev/97f42d9e462b
Part 14: Add MUrsh::NewWasm for MUrsh users in wasm code. r=jandem
https://hg.mozilla.org/integration/autoland/rev/cfdd8f0aa45c
Part 15: Set specialisation within MBinaryBitwiseInstruction constructor calls. r=jandem
https://hg.mozilla.org/integration/autoland/rev/1d50b0cf7158
Part 16: Remove MBinaryBitwiseInstruction::specializeAs. r=jandem
https://hg.mozilla.org/integration/autoland/rev/e3738b70e270
Part 17: Remove MBinaryBitwiseInstruction which handles non-number specialisations. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7a74700f4e86
Part 18: Remove no longer used codegen for non-number binary bitwse instructions. r=jandem
https://hg.mozilla.org/integration/autoland/rev/80d90187293b
Part 19: Don't call the always-inline UrshOperation in some places. r=jandem
https://hg.mozilla.org/integration/autoland/rev/5b653e25e757
Part 20: Change MBitNot to always specialise to Int32. r=jandem

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/43db59ebd874
https://hg.mozilla.org/mozilla-central/rev/b801d8007636
https://hg.mozilla.org/mozilla-central/rev/bd3e77b5b2e9
https://hg.mozilla.org/mozilla-central/rev/2ba73b4b8fd7
https://hg.mozilla.org/mozilla-central/rev/fba058bb2204
https://hg.mozilla.org/mozilla-central/rev/45396902c724
https://hg.mozilla.org/mozilla-central/rev/f37a78d23533
https://hg.mozilla.org/mozilla-central/rev/e14f372e1c71
https://hg.mozilla.org/mozilla-central/rev/13791501ae5d
https://hg.mozilla.org/mozilla-central/rev/d4fa8213b437
https://hg.mozilla.org/mozilla-central/rev/5b587fcde948
https://hg.mozilla.org/mozilla-central/rev/40a10f393143
https://hg.mozilla.org/mozilla-central/rev/4802a1f03448
https://hg.mozilla.org/mozilla-central/rev/97f42d9e462b
https://hg.mozilla.org/mozilla-central/rev/cfdd8f0aa45c
https://hg.mozilla.org/mozilla-central/rev/1d50b0cf7158
https://hg.mozilla.org/mozilla-central/rev/e3738b70e270
https://hg.mozilla.org/mozilla-central/rev/7a74700f4e86
https://hg.mozilla.org/mozilla-central/rev/80d90187293b
https://hg.mozilla.org/mozilla-central/rev/5b653e25e757