Closed Bug 903501 Opened 6 years ago Closed 6 years ago

reduce debug spew from compiling js/src/ion (jit) on ARM

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: froydnj, Unassigned)

Details

Attachments

(7 files)

Compiling js/src/ion on ARM spews tons of warnings.  The short patch series I'm about
to submit addresses most of the easy ones.  In particular, it eliminates warnings in
the header files, which cuts down on the spew quite a bit.

Apologies that these are from js/src/ion; I'm working off of central here.
Simple enough.
Attachment #788218 - Flags: review?(jdemooij)
The problem here is that size() returns uint32_t, so promotes the addition to unsigned,
which causes the warning.  I figured it was better to cast size() down to int32_t and
use that.
Attachment #788220 - Flags: review?(jdemooij)
-Wreorder complains if class members are not initialized in the order they are
declared.  Fixup BufferSlice to comply with our warning overlords.
Attachment #788222 - Flags: review?(jdemooij)
Not sure if the was supposed to use them or not.  I opted for deleting them; feel
free to tell me that I should have replaced, e.g. r0 with reg_code in the following
code and I'll do that instead.
Attachment #788225 - Flags: review?(jdemooij)
Attachment #788218 - Flags: review?(jdemooij) → review+
Attachment #788220 - Flags: review?(jdemooij) → review+
The discussion and patch in bug 898936 may be useful here.
Attachment #788221 - Flags: review?(jdemooij) → review+
Attachment #788222 - Flags: review?(jdemooij) → review+
Comment on attachment 788223 [details] [diff] [review]
part 5 - mark variables as DebugOnly in Assembler-arm.cpp

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

::: js/src/ion/arm/Assembler-arm.cpp
@@ +2536,5 @@
>      Instruction *ptr = (Instruction *) label.raw();
>      InstructionIterator iter(ptr);
>      Register dest;
>      Assembler::RelocStyle rs;
> +    DebugOnly<const uint32_t *> val = getPtr32Target(&iter, &dest, &rs);

Can you remove |val| and move the getPtr32Target() call into the JS_ASSERT? Avoids doing unnecessary work in opt builds (the call shouldn't have any side effects).
Attachment #788223 - Flags: review?(jdemooij) → review+
Attachment #788224 - Flags: review?(jdemooij) → review+
Comment on attachment 788225 [details] [diff] [review]
part 7 - delete unused variables in IonRuntime::generateEnterJIT

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

I think using the registers directly is fine. While you're here, can you also remove the #if 0 code in this function? I don't expect us to uncomment that anytime soon and it's just making things look more complicated.

Great patches, thank you for doing this!

::: js/src/ion/arm/Trampoline-arm.cpp
@@ +102,5 @@
>   */
>  IonCode *
>  IonRuntime::generateEnterJIT(JSContext *cx, EnterJitType type)
>  {
> +    DebugOnly<const Register> reg_frame = r3;

You can just remove reg_frame as well and use r3 directly in the assert below.
Attachment #788225 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #9)
> Comment on attachment 788223 [details] [diff] [review]
> part 5 - mark variables as DebugOnly in Assembler-arm.cpp
> 
> Review of attachment 788223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/arm/Assembler-arm.cpp
> @@ +2536,5 @@
> >      Instruction *ptr = (Instruction *) label.raw();
> >      InstructionIterator iter(ptr);
> >      Register dest;
> >      Assembler::RelocStyle rs;
> > +    DebugOnly<const uint32_t *> val = getPtr32Target(&iter, &dest, &rs);
> 
> Can you remove |val| and move the getPtr32Target() call into the JS_ASSERT?
> Avoids doing unnecessary work in opt builds (the call shouldn't have any
> side effects).

I don't think that will work very well; the getPtr32Target call is needed to initialize |dest| and |rs| for the call to ma_movPatchable...isn't it?
(In reply to Nathan Froyd (:froydnj) from comment #11)
> I don't think that will work very well; the getPtr32Target call is needed to
> initialize |dest| and |rs| for the call to ma_movPatchable...isn't it?

Oops, yes you're right, ignore that comment :)
And a followup fix because I didn't rebase over to inbound properly:

https://hg.mozilla.org/integration/mozilla-inbound/rev/782e74f1c4c6
You need to log in before you can comment on or make changes to this bug.