Closed Bug 797970 Opened 7 years ago Closed 7 years ago

IonMonkey: cleanup

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: evilpie, Assigned: evilpie)

Details

(Whiteboard: [ion:t])

Attachments

(1 file, 3 obsolete files)

First patch renames int32 -> int32_t. And removes two unnecessary headers.
Attachment #668097 - Flags: review?(dvander)
Attached patch wip remove NULL (obsolete) — Splinter Review
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #668135 - Attachment is obsolete: true
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?
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.
Whiteboard: [ion:t]
Attachment #668097 - Attachment is patch: false
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.
Attachment #668097 - Flags: review?(dvander) → review?(jdemooij)
Attachment #668097 - Attachment is patch: true
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.
Attachment #668097 - Flags: review?(jdemooij)
Here it is!
Attachment #668097 - Attachment is obsolete: true
Attachment #668145 - Attachment is obsolete: true
Attachment #686091 - Flags: review?(jdemooij)
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.
Attachment #686091 - Flags: review?(jdemooij) → review+
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
https://hg.mozilla.org/mozilla-central/rev/3b419a7a354c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Sweet!  uint32_t highlights the same as other builtin types in vim, uint32 did not.
(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.  ;-)
You need to log in before you can comment on or make changes to this bug.