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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - delete unused |sigil| variable in AssemblerBuffer::getInst

Simple enough.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - eliminate signed-unsigned comparison warning in AssemblerBuffer::getInst

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.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - mark DebugOnly variables in AssemblerBufferWithConstantPool methods

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - initialize BufferSlice members in a form amenable to -Wreorder

-Wreorder complains if class members are not initialized in the order they are
declared.  Fixup BufferSlice to comply with our warning overlords.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - mark variables as DebugOnly in Assembler-arm.cpp

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 6 - mark DebugOnly variable in TryToSpecializeBinaryArithOp

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 7 - delete unused variables in IonRuntime::generateEnterJIT

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.

Comment 8

[Terrence Cole [:terrence]]

The discussion and patch in bug 898936 may be useful here.

Comment 9

[Jan de Mooij [:jandem]]

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).

Comment 10

[Jan de Mooij [:jandem]]

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.

Comment 11

[Nathan Froyd [:froydnj]]

(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?

Comment 12

[Jan de Mooij [:jandem]]

(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 :)

Comment 13

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/000a3ebae6d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/645564aa4a30
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4c8614250a
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5d40e468d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00d22fc0840
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a49f03aa849
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d42eb2f5b63

Comment 14

[Nathan Froyd [:froydnj]]

And a followup fix because I didn't rebase over to inbound properly:

https://hg.mozilla.org/integration/mozilla-inbound/rev/782e74f1c4c6

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/000a3ebae6d9
https://hg.mozilla.org/mozilla-central/rev/645564aa4a30
https://hg.mozilla.org/mozilla-central/rev/2b4c8614250a
https://hg.mozilla.org/mozilla-central/rev/4b5d40e468d5
https://hg.mozilla.org/mozilla-central/rev/f00d22fc0840
https://hg.mozilla.org/mozilla-central/rev/3a49f03aa849
https://hg.mozilla.org/mozilla-central/rev/0d42eb2f5b63
https://hg.mozilla.org/mozilla-central/rev/782e74f1c4c6