Closed Bug 1058685 Opened 5 years ago Closed 5 years ago

IonMonkey MIPS: Fix warnings when building for MIPS.

Categories

(Core :: JavaScript Engine: JIT, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: rankov, Assigned: rankov)

Details

Attachments

(2 files, 1 obsolete file)

There are a number of warnings that can be easily avoided when building for MIPS simulator.
Assignee: nobody → branislav.rankov
Status: NEW → ASSIGNED
Attached patch Warnings.patchSplinter Review
Attachment #8479824 - Flags: review?(benj)
Attached patch Clean-up-LAsmJSLoadFuncPtr.patch (obsolete) — Splinter Review
Attachment #8479826 - Flags: review?(benj)
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.
Attachment #8479824 - Flags: review?(benj) → review+
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>
Attachment #8479826 - Flags: review?(benj) → review+
(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.
Fixed LInstructionHelper<1, 1, 0>.
Carry review from previous patch.
Attachment #8479826 - Attachment is obsolete: true
Attachment #8479844 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/5a908388adc0
https://hg.mozilla.org/mozilla-central/rev/5c977e86af7b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.