821649 - IonMonkey: (ARM) Add in codegen spew

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/codegenSpew-r0.patch

the jit inspector hooks into the codegen spew which is currently non-existent on ARM.  this fixes it.

Comment 1

[Jacob Bramley [:jbramley]]

Comment on attachment 692239 [details] [diff] [review]
/home/mrosenberg/patches/codegenSpew-r0.patch

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

Lots of small issues again, hence r-.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +34,1 @@
>  

There are no spaces between these functions, except here. Spaces make things easier to read, for me, but consistency is valuable.

@@ +167,5 @@
> +        sprintf(tmp, "%s", Registers::GetName(Registers::Code(rrs.RS)));
> +    } else {
> +        datastore::RIS ris = datastore::RIS::decode(r.ShiftAmount);
> +        if (ris.ShiftAmount == 0 && r.Type == LSL) {
> +            sprintf(buff, ", %s%s", iu, Registers::GetName(Registers::Code(r.RM)));

'buff' will be overwritten. In Op2Reg::ToString, you returned at this point, and I think you should here too.

@@ +177,5 @@
> +}
> +char *
> +DTRAddr::ToString(Index mode, char *buff) const
> +{
> +    char cb[6], coff[30];

I'd make these buffers much bigger, so you don't get problems with debug values (like "INVALID"), should they turn up. This is debug code that doesn't recurse, so the extra stack usage shouldn't matter. 128 bytes is probably a decent size so you can be sure you won't overflow.

The same applies to other functions too.

@@ +178,5 @@
> +char *
> +DTRAddr::ToString(Index mode, char *buff) const
> +{
> +    char cb[6], coff[30];
> +    const char*fmt;

There should be another space in there.

@@ +1385,5 @@
>  Assembler::as_alu(Register dest, Register src1, Operand2 op2,
>                  ALUOp op, SetCond_ sc, Condition c)
>  {
> +    char cd[6], cs1[6], cop2[16];
> +    log("%s%s%s %s%s%s", opToString(op), condToString(c), scToString(op, sc),

In UAL syntax (which is what you should be printing), the 'S' flag comes before the condition flag. For example, print "addsne" rather than "addnes".

@@ +1703,5 @@
>  BufferOffset
>  Assembler::as_Imm32Pool(Register dest, uint32_t value, ARMBuffer::PoolEntry *pe, Condition c)
>  {
> +    char cd[6];
> +    log("ldr%s %s imm #(%p)", condToString(c), regToString(dest,cd), value);

Conventional syntax would be like "ldr r0, =0x12345678", where 0x12345678 is the literal. This is used by the GNU assembler so it's widely-recognized.

@@ +1712,5 @@
>  
>  BufferOffset
>  Assembler::as_BranchPool(uint32_t value, RepatchLabel *label, ARMBuffer::PoolEntry *pe, Condition c)
>  {
> +    log("ldr%s pc, imm #(%p)", condToString(c), value);

Ditto.

@@ +1738,2 @@
>      JS_ASSERT(dest.isDouble());
> +    log("vldr%s %s imm #(%f)", condToString(c), vregToString(dest, cd), value);

Ditto, though the GNU assembler doesn't use the ldr= syntax for vldr. That is, "vldr d0, =1.5f" would not assemble, but the syntax is clear by extension.

@@ +1815,5 @@
>  BufferOffset
>  Assembler::as_bx(Register r, Condition c, bool isPatchable)
>  {
> +    char cd[6];
> +    log("b%s %s", condToString(c), regToStringTerm(r, cd));

"bx", not "b".

@@ +1844,5 @@
>  
>  BufferOffset
>  Assembler::as_b(Label *l, Condition c, bool isPatchable)
>  {
> +    log("b%s", condToString(c));

To where? Can you put some information about the label in there?

@@ +1888,5 @@
>  BufferOffset
>  Assembler::as_blx(Register r, Condition c)
>  {
> +    char cr[6];
> +    log("blx%s", condToString(c), regToStringTerm(r,cr));

Missing "%s".

@@ +1904,5 @@
>  
>  BufferOffset
>  Assembler::as_bl(Label *l, Condition c)
>  {
> +    log("bl%s", condToString(c));

Print something about the label, as for as_b.

@@ +1956,5 @@
>  Assembler::as_vfp_float(VFPRegister vd, VFPRegister vn, VFPRegister vm,
>                    VFPOp op, Condition c)
>  {
> +    char cd[6], cn[6], cm[6];
> +    log("%s%s %s %s %s", vfpToString(op),condToString(c), vregToString(vd, cd), vregToString(vn, cn), vregToStringTerm(vm, cm));

That's a really long line. There are others, but I won't pick them all out.

@@ +2067,5 @@
> +    char c1[6], c2[6], cm[6];
> +    if (f2c == FloatToCore)
> +        log("vmov%s %s%s%s", condToString(c), regToString(vt1,c1), regToString(vt2,c2), vregToStringTerm(vm,cm));
> +    else
> +        log("vmov%s %s%s%s", condToString(c), vregToString(vm,cm), regToString(vt1,c1), regToStringTerm(vt2,c2));

This won't work well for "vmov s0, r0". In that case, I think vt2 would be invalid, and wouldn't print anything, but regToString(vt1,c1) would add a trailing comma, so you'd get "vmov s0, r0, ".

@@ +2153,5 @@
>  
>  BufferOffset
>  Assembler::as_vcvtFixed(VFPRegister vd, bool isSigned, uint32_t fixedPoint, bool toFixed, Condition c)
>  {
> +    log("vcvt.fixed");

Print some arguments, or a TODO, or something.

@@ +2174,5 @@
>  {
>      vfp_size sz = vd.isDouble() ? isDouble : isSingle;
> +    char cd[6];
> +    char ca[30];
> +    log("%s%s%s %s%s", ls == IsLoad ? "vldr" : "vstr", vd.isDouble() ? ".f64" : ".f32", condToString(c),

The condition comes before the type specifier, like this:

vldreq.f64 d0, [...]

Actually, the type specifier is superfluous for vldr and vstr so you could miss it out completely.

@@ +2207,1 @@
>      // Don't know how to handle this right now.

It's not related to this patch, but immediates for S registers are just the same as for D registers. The full range of encodable immediates can fit into either a float or a double.

@@ +2237,5 @@
>  
>  void
>  Assembler::bind(Label *label, BufferOffset boff)
>  {
> +    log("binding label");

Which label? Can you print an ID of some kind, so that B and BX instructions can be linked with their labels by a user?

::: js/src/ion/arm/Assembler-arm.h
@@ +134,5 @@
> +        switch(kind) {
> +          case Double: return ".F64";
> +          case Single: return ".F32";
> +          case UInt: return ".U32";
> +          case Int: return ".I32";

We print instructions in lower case, so should these be lower case too? Actually, ".f32" and ".f64" are hard-coded in at least one place.

@@ +418,5 @@
>      }
> +    static Reg decode(uint32_t arg) {
> +        return *(Reg*)(&arg);
> +    }
> +    

Trailing whitespace.

@@ +544,5 @@
>          JS_ASSERT(ShiftAmount == imm);
>      }
> +    static RIS decode(uint32_t arg) {
> +        RIS ret = *(RIS*)(&arg);
> +        return ret;

That violates pointer aliasing rules. Do this instead:

  RIS ret;
  ASSERT(sizeof(ret) == sizeof(arg));
  memcpy(&ret, &arg, sizeof(ret));
  return ret;

It should generate the same code (which in this case is no code), but memcpy is safe for strict aliasing rules.

@@ +565,5 @@
>      uint32_t encode () {
>          return RS << 1;
>      }
> +    static RRS decode(uint32_t arg) {
> +        RRS ret = *(RRS*)(&arg);

Same comment as for RIS decode.

@@ +658,5 @@
>      Imm8(uint32_t imm)
>        : Operand2(encodeImm(imm))
>      { }
> +    char *ToString(char *buff) const {
> +        uint32_t shift = 0xf00 & oper;

You need to shift down the shift encoding. It's a multiple of two, so you need "(0xf00 & oper) >> 7".

@@ +660,5 @@
>      { }
> +    char *ToString(char *buff) const {
> +        uint32_t shift = 0xf00 & oper;
> +        uint32_t base = 0xff & oper;
> +        sprintf(buff, "#%d", base >> shift | base << (32 -shift));

Careful! The behaviour of the shift operations is undefined past the size of the type. So, if shift is 0, you have "base << 32" in there. Similarly, if shift is 32, you have "base >> 0". This isn't just me being pedantic about the language, since real bugs come from this. Most notably, on x86 the shift is modulo-32 so "base << 32" means "base << 0". On ARM, register-shifts are modulo-256 (and immediate shifts can only encode sensible ranges).

Bit-rotate operations are awkward to implement safely in C for this reason.

@@ +740,5 @@
>        : DtrOff(datastore::Imm12Data(abs(imm)), imm >= 0 ? IsUp : IsDown)
>      {
>          JS_ASSERT((imm < 4096) && (imm > -4096));
>      }
> +    char *ToString(char *buff, const char *iu) const {

Why isn't this alongside DtrOffReg::ToString? I thought it was missing until I got to this point in the patch.

@@ +798,5 @@
>  
>      uint32_t encode() {
>          return data;
>      }
> +    Register getBase() const {

Why only this one? There are many methods which could be "const", and it's rarely a bad idea to do it.

@@ +903,5 @@
>      VFPAddr(Register base, VFPOff off)
>        : data(RN(base) | off.encode())
>      { }
> +    Register getBase() const {
> +        return toRN(*(Instruction*)&(data));

Use memcpy, as described previously.

@@ +1303,5 @@
>                  m_buffer.fail_oom();
>                  return;
>              }
>          }
> +        fprintf(stderr, "printer is initially: %p\n", printer);

I think this is the debug printf you mentioned.

@@ +1346,5 @@
>  
>      bool oom() const;
>  
>      void setPrinter(Sprinter *sp) {
> +        fprintf(stderr, "setting printer to: %p\n", sp);

More debug.

Comment 2

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/codegenSpew-r1.patch

Fixes all of the nits, some of them in less-than-ideal ways

Comment 3

[Marty Rosenberg [:mjrosenb]]

oops, just caught a bug where I forgot to set MustZero to zero, not actually a bug, but it causes us to throw an assertion.  I'll commit it along with everything else.

Comment 4

[Jacob Bramley [:jbramley]]

Comment on attachment 692990 [details] [diff] [review]
/home/mrosenberg/patches/codegenSpew-r1.patch

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

I have a couple of comments, but it basically looks good.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +16,5 @@
>  using namespace js;
>  using namespace js::ion;
> +char *preRegToString(Register r, char *buf, const char *rest) {
> +    if (r == InvalidReg) {
> +        strcpy(buf, rest);

If r == Invalid and rest == NULL, this will break. In practice, rest is never NULL, but can be an empty string. If you remove the NULL check a couple of lines down (but maybe ASSERT it if you want), then this function would look safer, even if we never actually pass NULL.

@@ +31,5 @@
> +    if (r.isMissing()) {
> +        strcpy(buf, rest);
> +        return buf;
> +    }
> +    if (rest != NULL && *rest != '\0')

Same comment as for preRegToString.

@@ +171,5 @@
> +{
> +    datastore::Reg r = datastore::Reg::decode(data);
> +    char tmp[128];
> +    if (r.RRS) {
> +        datastore::RRS rrs = datastore::RRS::decode(r.ShiftAmount);

If I understand correctly, r.ShiftAmount actually holds the shift register ("Rs" in the ARM ARM), and the bottom bit must be 0. Is that correct? If so, it's a bit weird, though I don't really know what to suggest other than reworking the way that the Reg struct works. You already have a comment in there saying that you'd like to have a more sensible encoding, but I think working that out would be pretty tricky, and this patch isn't the place for it.

::: js/src/ion/arm/Assembler-arm.h
@@ +416,5 @@
>      uint32_t encode() {
>          return RM | RRS << 4 | Type << 5 | ShiftAmount << 7;
>      }
> +    static Reg decode(uint32_t arg) {
> +        return *(Reg*)(&arg);

This violates pointer-aliasing rules too.

@@ +567,5 @@
> +    static RRS decode(uint32_t arg) {
> +        RRS ret;
> +        JS_ASSERT(sizeof(ret) == sizeof(arg));
> +        memcpy(&ret, &arg, sizeof(ret));
> +        return ret;

Actually, seeing your fix to RIS, this looks a little cumbersome. The following is probably better:

RIS ret((arg >> 1) & 0xf);
return ret;

I'll let you decide what is best here.

@@ +664,5 @@
>      { }
> +    char *ToString(char *buff) const {
> +        uint32_t shift = (0xf00 & oper) >> 7;
> +        uint32_t base = 0xff & oper;
> +        sprintf(buff, "#%d", base >> shift | base << ((32 - shift) & 0x1f));

This is still technically broken if shift is 32. You should extend your mask to the first shift too:

... base >> (shift & 0x1f) | base << ((32 - shift) & 0x1f)

Since I'm being pedantic, it's also broken in that the cast from uint32_t to int32_t (for the printf) is implementation-defined for negative values. I suspect that we can live with that, however.

Comment 5

[Marty Rosenberg [:mjrosenb]]

(In reply to Jacob Bramley [:jbramley] from comment #4)


> @@ +171,5 @@
> > +{
> > +    datastore::Reg r = datastore::Reg::decode(data);
> > +    char tmp[128];
> > +    if (r.RRS) {
> > +        datastore::RRS rrs = datastore::RRS::decode(r.ShiftAmount);
> 
> If I understand correctly, r.ShiftAmount actually holds the shift register
> ("Rs" in the ARM ARM), and the bottom bit must be 0. Is that correct? If so,
> it's a bit weird, though I don't really know what to suggest other than
> reworking the way that the Reg struct works. You already have a comment in
> there saying that you'd like to have a more sensible encoding, but I think
> working that out would be pretty tricky, and this patch isn't the place for
> it.
> 
> ::: js/src/ion/arm/Assembler-arm.h
> @@ +416,5 @@
> >      uint32_t encode() {
> >          return RM | RRS << 4 | Type << 5 | ShiftAmount << 7;
> >      }
> > +    static Reg decode(uint32_t arg) {
> > +        return *(Reg*)(&arg);
> 
> This violates pointer-aliasing rules too.
> 
I didn't make it a memcpy because then I'd need to be able to create a Reg without passing in the 4 arguments.  I opted to just make a constructor that takes a single argument and does the memcpy.


> @@ +664,5 @@
> >      { }
> > +    char *ToString(char *buff) const {
> > +        uint32_t shift = (0xf00 & oper) >> 7;
> > +        uint32_t base = 0xff & oper;
> > +        sprintf(buff, "#%d", base >> shift | base << ((32 - shift) & 0x1f));
> 
> This is still technically broken if shift is 32. You should extend your mask
> to the first shift too:
> 
> ... base >> (shift & 0x1f) | base << ((32 - shift) & 0x1f)
> 
> Since I'm being pedantic, it's also broken in that the cast from uint32_t to
> int32_t (for the printf) is implementation-defined for negative values. I
> suspect that we can live with that, however.
it should be impossible to get shift == 32, since
js> 0xf00 >> 7
30
and now that I think about it, if you interpret x << 32 as 0 or as x, it shouldn't affect the computation, but nonetheless, having undefined behavior is bad.

Comment 6

[Marty Rosenberg [:mjrosenb]]

Attachment: /home/mrosenberg/patches/codegenSpew-r2.patch

Ripping out the bit where I added an extra field to the Label structure.

Comment 7

[Jacob Bramley [:jbramley]]

> it should be impossible to get shift == 32, since
> js> 0xf00 >> 7
> 30
> and now that I think about it, if you interpret x << 32 as 0 or as x, it
> shouldn't affect the computation, but nonetheless, having undefined behavior
> is bad.

Oh, of course, sorry. The code is fine!