Closed Bug 1627618 Opened 10 months ago Closed 10 months ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox76 --- wontfix
firefox77 --- fixed

People

(Reporter: anba, Assigned: anba)

References

Details

Attachments

(20 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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.

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

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

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

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

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

Depends on D69783

Set the specialisation and the return type directly in MBinaryArithInstruction.

Also assert that the specialisation is definitely a number type.

Depends on D69785

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

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

Depends on D69788

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

Depends on D69789

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

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

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

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

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

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

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

Depends on D69796

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

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

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

Pushed by ccoroiu@mozilla.com:
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
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.