797970 - IonMonkey: cleanup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: Use proper stdint types even in ion/

First patch renames int32 -> int32_t. And removes two unnecessary headers.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: wip remove NULL

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: v1

I *think* I fixed all of the wrong conversations I did in the previous patch. But looking at nullptr for a certain time makes you dull.

Comment 3

[Nicolas B. Pierron [:nbp]]

This goes against the coding style we have on IonMonkey, do you have any reasons which might justify changing all?  and changing all of them at once instead of incrementally?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

I explicitly opted out of C99 types when JS took them in, because I dislike hitting shift for the _t. If other people like the C99 types though, I won't object - it should be up to the IM team members or JS module owner.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 668097 [details] [diff] [review]
Use proper stdint types even in ion/

I think the people this affects most now are the baseline compiler folks, so switching review to Jan.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 668097 [details] [diff] [review]
Use proper stdint types even in ion/

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

Tom, can you post an updated patch? I'm fine with taking this, when it merges to the IonMonkey branch it's a matter of fixing all compile errors. Shouldn't take too much time.

Comment 7

[Tom S. (please needinfo tschuster)]

Attachment: v2 of stdint types

Here it is!

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 686091 [details] [diff] [review]
v2 of stdint types

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

Quickly skimmed this and looks good. r=me with comments addressed, but please grep the patch for comment changes.

::: js/src/ion/CodeGenerator.cpp
@@ +531,5 @@
>  bool
>  CodeGenerator::visitStoreSlotV(LStoreSlotV *store)
>  {
>      Register base = ToRegister(store->slots());
> +    int32_t offset  = store->mir()->slot() * sizeof(Value);

Remove extra space before =

@@ +3232,5 @@
>  
>      return true;
>  }
>  
> +// An out-of-line path to convert a boxed int32_t to a double.

Revert.

@@ +3257,5 @@
>  {
>      const ValueOperand box = ToValue(lir, LUnboxDouble::Input);
>      const LDefinition *result = lir->output();
>  
> +    // Out-of-line path to convert int32_t to double or bailout

And here.

@@ +4102,4 @@
>  {
>      Register input = ToRegister(lir->input());
>      Register output = ToRegister(lir->output());
> +    masm.clampIntToUint8_t(input, output);

And here.

@@ +4146,5 @@
>      masm.jump(&done);
>  
>      masm.bind(&isDouble);
>      masm.unboxDouble(input, tempFloat);
> +    masm.clampDoubleToUint8_t(tempFloat, output);

And here.

::: js/src/ion/CodeGenerator.h
@@ +154,5 @@
>      bool visitLoadTypedArrayElementHole(LLoadTypedArrayElementHole *lir);
>      bool visitStoreTypedArrayElement(LStoreTypedArrayElement *lir);
> +    bool visitClampIToUint8_t(LClampIToUint8_t *lir);
> +    bool visitClampDToUint8_t(LClampDToUint8_t *lir);
> +    bool visitClampVToUint8_t(LClampVToUint8_t *lir);

Revert.

::: js/src/ion/IonMacroAssembler.cpp
@@ +231,5 @@
>        case TypedArray::TYPE_UINT32:
>          load32(src, dest.scratchReg());
>          test32(dest.scratchReg(), dest.scratchReg());
>          if (allowDouble) {
> +            // If the value fits in an int32_t, store an int32_t type tag.

Revert.

@@ +246,5 @@
>                  boxDouble(ScratchFloatReg, dest);
>              }
>              bind(&done);
>          } else {
> +            // Bailout if the value does not fit in an int32_t.

Same here.

@@ +269,5 @@
>                                                   bool allowDouble, Label *fail);
>  
>  // Note: this function clobbers the input register.
>  void
> +MacroAssembler::clampDoubleToUint8_t(FloatRegister input, Register output)

And here.

::: js/src/ion/IonMacroAssembler.h
@@ +472,5 @@
>      }
>  
>      // Inline version of js_TypedArray_uint8_clamp_double.
>      // This function clobbers the input register.
> +    void clampDoubleToUint8_t(FloatRegister input, Register output);

Don't rename this one.

::: js/src/ion/LIR-Common.h
@@ +2420,5 @@
>          return getOperand(2);
>      }
>  };
>  
> +class LClampIToUint8_t : public LInstructionHelper<1, 1, 0>

Don't rename this.

@@ +2430,5 @@
>          setOperand(0, in);
>      }
>  };
>  
> +class LClampDToUint8_t : public LInstructionHelper<1, 1, 1>

And this.

@@ +2441,5 @@
>          setTemp(0, temp);
>      }
>  };
>  
> +class LClampVToUint8_t : public LInstructionHelper<1, BOX_PIECES, 1>

Same here.

::: js/src/ion/LOpcodes.h
@@ +132,5 @@
>      _(LoadTypedArrayElementHole)    \
>      _(StoreTypedArrayElement)       \
> +    _(ClampIToUint8_t)                \
> +    _(ClampDToUint8_t)                \
> +    _(ClampVToUint8_t)                \

Revert these.

::: js/src/ion/MIR.cpp
@@ +622,5 @@
>                  if (!div->canBeNegativeZero())
>                      return true;
>              } else if (operand->isToInt32()) {
> +                MToInt32 *int32_t = operand->toToInt32();
> +                if (!int32_t->canBeNegativeZero())

Heh, please revert.

@@ +957,5 @@
>  void
>  MBinaryArithInstruction::infer(JSContext *cx, const TypeOracle::BinaryTypes &b)
>  {
>      // Retrieve type information of lhs and rhs
> +    // Rhs is defaulted to int32_t first, 

And this.

@@ +997,5 @@
>              return;
>          }
>      }
>  
> +    // Don't specialize as int32_t if one of the operands is undefined,

And this.

@@ +1104,5 @@
>          }
>      }
>  
>      if (jsop() == JSOP_STRICTEQ || jsop() == JSOP_STRICTNE) {
> +        // bool/bool case got an int32_t specialization earlier.

Ditto. Can you do a grep on the patch to filter all lines starting with - or + and containing "//" or "*" and check these manually?

::: js/src/ion/MIR.h
@@ +70,5 @@
>  {
>      friend class MDefinition;
>  
>      MNode *node_;           // The node that is using this operand.
> +    uint32_t index_;          // The index of this operand in its owner.

Nit: align comment.

@@ +1749,5 @@
>          return AliasSet::None();
>      }
>  };
>  
> +// Converts a value or typed input to a truncated int32_t, for use with bitwise

Revert.

@@ +2469,5 @@
>      // Annotation the result could be a negative zero
>      // and we need to guard this during execution.
>      bool canBeNegativeZero_;
>  
> +    // Annotation the result of this Mul is only used in int32_t domain

Revert.

Comment 9

[Tom S. (please needinfo tschuster)]

I think I found all wrong changes. But when you are bored feel free to check the commit.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b419a7a354c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3b419a7a354c

Comment 11

[Luke Wagner [:luke]]

Sweet!  uint32_t highlights the same as other builtin types in vim, uint32 did not.

Comment 12

[(no longer active)]

(In reply to comment #11)
> Sweet!  uint32_t highlights the same as other builtin types in vim, uint32 did
> not.

Well, uint32_t is a real type.  ;-)