1058685 - IonMonkey MIPS: Fix warnings when building for MIPS.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Branislav Rankov [:rankov]]

There are a number of warnings that can be easily avoided when building for MIPS simulator.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: Warnings.patch

Comment 2

[Branislav Rankov [:rankov]]

Attachment: Clean-up-LAsmJSLoadFuncPtr.patch

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8479824 [details] [diff] [review]
Warnings.patch

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

Looks good.

::: js/src/jit/mips/Assembler-mips.cpp
@@ +1577,5 @@
>          else
>              branch.setRT((RTField) ((rt | 0x1) << RTShift));
>          return branch;
> +      default:
> +        MOZ_CRASH("Error creating long branch.");

You can also have a list of explicit case here for other possible values, that just |break;| and then MOZ_CRASH outside the switch, which will let the compiler emit warnings if a case is not explicitly handled. Up to you though ;)

::: js/src/jit/mips/Assembler-mips.h
@@ +239,5 @@
>  static const uint32_t FunctionMask = ((1 << FunctionBits) - 1) << FunctionShift;
>  static const uint32_t RegMask = Registers::Total - 1;
>  static const uint32_t StackAlignmentMask = StackAlignment - 1;
>  
> +static const uint32_t MAX_BREAK_CODE = 1024 - 1;

I assume all the int32_t -> uint32_t changes are due to signed vs unsigned comparisons.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8479826 [details] [diff] [review]
Clean-up-LAsmJSLoadFuncPtr.patch

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

Cool

::: js/src/jit/mips/LIR-mips.h
@@ +374,5 @@
>          return mir_->toMod();
>      }
>  };
>  
>  class LAsmJSLoadFuncPtr : public LInstructionHelper<1, 1, 1>

If you don't need the temporary, I think you can modify this to LInstructionHelper<1, 1, 0>

Comment 5

[Branislav Rankov [:rankov]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)

> You can also have a list of explicit case here for other possible values,
> that just |break;| and then MOZ_CRASH outside the switch, which will let the
> compiler emit warnings if a case is not explicitly handled. Up to you though
> ;)

There is a really long list of opcodes. I would rather not list them all here.

> I assume all the int32_t -> uint32_t changes are due to signed vs unsigned
> comparisons.

This is correct.

Comment 6

[Branislav Rankov [:rankov]]

Attachment: Clean-up-LAsmJSLoadFuncPtr.patch

Fixed LInstructionHelper<1, 1, 0>.
Carry review from previous patch.

Comment 7

[Branislav Rankov [:rankov]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a908388adc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c977e86af7b

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/5a908388adc0
https://hg.mozilla.org/mozilla-central/rev/5c977e86af7b