Closed Bug 1435249 Opened 2 years ago Closed 2 years ago

Improve CMOVcc instruction encoding.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

Currently we have one opcode per condition flag, most cmov instruction can actually share the same way we compute the opcode as the Jcc instruction encoding.
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Status: ASSIGNED → NEW
Priority: -- → P1
Extract CMOV patch from Bug 1433111 patch, and apply nits from IRC:
 - MOZ_CRASH (MOZ_RELEASE_ASSERT) if the CPU does not support cmov instructions.
Attachment #8947799 - Flags: review?(jdemooij)
Comment on attachment 8947799 [details] [diff] [review]
Generalized x86/x64 cmov encoding.

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

Very nice, thanks!

::: js/src/jit/x64/Assembler-x64.h
@@ +464,5 @@
>  
> +    void cmovCCq(Condition cond, const Operand& src, Register dest) {
> +        X86Encoding::Condition cc = static_cast<X86Encoding::Condition>(cond);
> +        switch (src.kind()) {
> +        case Operand::REG:

Nit: indent case labels with 2 more spaces, also in cmovCCl

::: js/src/jit/x86-shared/Assembler-x86-shared.cpp
@@ +266,5 @@
>  CPUInfo::SSEVersion CPUInfo::maxSSEVersion = UnknownSSE;
>  CPUInfo::SSEVersion CPUInfo::maxEnabledSSEVersion = UnknownSSE;
>  bool CPUInfo::avxPresent = false;
>  bool CPUInfo::avxEnabled = false;
> +bool CPUInfo::cmovPresent = false;

Nit: can remove this line
Attachment #8947799 - Flags: review?(jdemooij) → review+
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61d73dd6b95f
Generalized x86/x64 cmov encoding. r=jandem
https://hg.mozilla.org/mozilla-central/rev/61d73dd6b95f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8947799 [details] [diff] [review]
Generalized x86/x64 cmov encoding.

See bug 1431173 comment 6.
Attachment #8947799 - Flags: approval-mozilla-beta?
Comment on attachment 8947799 [details] [diff] [review]
Generalized x86/x64 cmov encoding.

Spectre-related fix. Taking for 59b8.
Attachment #8947799 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1437510
You need to log in before you can comment on or make changes to this bug.