1390214 - Encoding-x86-shared.h:318:12: warning: comparison of two values with different enumeration types in switch statement

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Daniel Holbert [:dholbert]]

When building mozilla-central with clang trunk (version 6.0, locally built from revision 310772), I get the following build warning:

===========
js/src/jit/x86-shared/Encoding-x86-shared.h:318:12: warning: comparison of two values with different enumeration types in switch statement ('js::jit::X86Encoding::TwoByteOpcodeID' and 'js::jit::X86Encoding::ThreeByteOpcodeID') [-Wenum-compare-switch]

       case OP3_PEXTRD_EdVdqIb:
            ^~~~~~~~~~~~~~~~~~
===========

I think this might be a new warning in clang? Anyway, it looks kind of legit... Seems like a very odd use-case to have case statements for different enum types in the same switch block ( TwoByteOpcodeID vs ThreeByteOpcodeID ).  Here's the code in question, with a comment added on the problematic line:

> // Test whether the given opcode should be printed with its operands reversed.
> inline bool IsXMMReversedOperands(TwoByteOpcodeID opcode)
> {
>     switch (opcode) {
>       case OP2_MOVSD_WsdVsd: // also OP2_MOVPS_WpsVps
>       case OP2_MOVAPS_WsdVsd:
>       case OP2_MOVDQ_WdqVdq:
>       case OP3_PEXTRD_EdVdqIb: // *****CLANG WARNS FOR THIS ONE*****
>         return true;
>       default:
>         break;
>     }
>     return false;
> }

This definitely looks iffy, even beyond the switch statement warning, because the type of the function-param |opcode| is TwoByteOpcodeID, and OP3_PEXTRD_EdVdqIb is *not* that type.

Also, as a side note -- there *are* a couple of TwoByteOpcodeID enum-values with the same numeric value as "OP3_PEXTRD_EdVdqIb", FWIW -- not sure if that's problematic or not from the perspective of this mixed-enum-type switch statement. Specifically, there are:
> enum TwoByteOpcodeID {
>     [...]
>     OP2_MOVLHPS_VqUq    = 0x16,
>     OP2_MOVSHDUP_VpsWps = 0x16,

...vs.:

> enum ThreeByteOpcodeID {
>     [...]
>     OP3_PEXTRD_EdVdqIb  = 0x16,

So we might be accidentally returning "True" for those OP2_*** opcodes here.

HISTORICAL NOTE:
This line was added here:
https://hg.mozilla.org/mozilla-central/rev/736d53322a1d91210f65e66e8d5254ddd791a370#l3.33
as part of bug 1115752. Marking as dependency of that bug.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: threebyte-reversed-operands.patch

IsXMMReversedOperands isn't used for 3-byte opcode encodings, so this case can be removed.

Comment 3

[Dan Gohman [:sunfish]]

Comment on attachment 8902972 [details] [diff] [review]
threebyte-reversed-operands.patch

IsXMMReversedOperands isn't used for 3-byte opcodes, so this case can be removed.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5316b09380776ef8cc826dc6cc326f92f49d4965
Bug 1390214 - IonMonkey: Don't test for a 3-byte opcode in a 2-byte opcode predicate r=jandem

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5316b0938077
IonMonkey: Don't test for a 3-byte opcode in a 2-byte opcode predicate r=jandem

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Dan, I took the liberty to land it for you (with the ownership obviously) as it is breaking some CIs.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5316b0938077