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

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 788218 [details] [diff] [review]
part 1 - delete unused |sigil| variable in AssemblerBuffer::getInst

Simple enough.
Attachment #788218 - Flags: review?(jdemooij)
(Reporter)

Comment 2

4 years ago
Created attachment 788220 [details] [diff] [review]
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.
Attachment #788220 - Flags: review?(jdemooij)
(Reporter)

Comment 3

4 years ago
Created attachment 788221 [details] [diff] [review]
part 3 - mark DebugOnly variables in AssemblerBufferWithConstantPool methods
Attachment #788221 - Flags: review?(jdemooij)
(Reporter)

Comment 4

4 years ago
Created attachment 788222 [details] [diff] [review]
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.
Attachment #788222 - Flags: review?(jdemooij)
(Reporter)

Comment 5

4 years ago
Created attachment 788223 [details] [diff] [review]
part 5 - mark variables as DebugOnly in Assembler-arm.cpp
Attachment #788223 - Flags: review?(jdemooij)
(Reporter)

Comment 6

4 years ago
Created attachment 788224 [details] [diff] [review]
part 6 - mark DebugOnly variable in TryToSpecializeBinaryArithOp
Attachment #788224 - Flags: review?(jdemooij)
(Reporter)

Comment 7

4 years ago
Created attachment 788225 [details] [diff] [review]
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.
Attachment #788225 - Flags: review?(jdemooij)

Updated

4 years ago
Attachment #788218 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Attachment #788220 - Flags: review?(jdemooij) → review+
The discussion and patch in bug 898936 may be useful here.

Updated

4 years ago
Attachment #788221 - Flags: review?(jdemooij) → review+

Updated

4 years ago
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+

Updated

4 years ago
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+
(Reporter)

Comment 11

4 years ago
(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 :)
(Reporter)

Comment 13

4 years ago
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
(Reporter)

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/782e74f1c4c6
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.