Closed Bug 676933 Opened 13 years ago Closed 13 years ago

IonMonkey: implement BITNOT

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(2 files, 4 obsolete files)

Implement JSOP_BITNOT in ionmonkey.
Attached patch implement JSOP_BITNOT (obsolete) — Splinter Review
Assignee: general → hv1989
Attachment #551145 - Flags: review?(dvander)
Comment on attachment 551145 [details] [diff] [review]
implement JSOP_BITNOT

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

::: js/src/ion/IonLowering.cpp
@@ +195,5 @@
> +
> +    JS_ASSERT(input->type() == MIRType_Int32);
> +
> +    LUnaryBitOp *bitop = new LUnaryBitOp(op, useRegister(input));
> +    return defineReuseInput(bitop, ins);

I suspect ARM's version of this will look slightly different, wanting define() instead of defineReuseInput() since it tends to have separate src and dest registers.

We don't have to do it now but maybe we should think about having an abstraction here, either tweaking lowerForALU or introducing a Lowering-x86-shared? (because we don't have enough classes in our hierarchy :)

::: js/src/ion/MIR.cpp
@@ +375,5 @@
> +MUnaryBitwiseInstruction::infer(const TypeOracle::Unary &u)
> +{
> +    if (u.ival == MIRType_Object)
> +        specialization_ = MIRType_None;
> +    else {

House style is: If one sub-block gets braced, all should get braces.\:

  if (x) {
    ... one liner ...
  } else {
    ...
    ...
  }
Attachment #551145 - Flags: review?(dvander) → review+
Attached patch implement JSOP_BITNOT (obsolete) — Splinter Review
- added requested changes
- added the folding of bitnot
- I added lowerForALU(LInstructionHelper<1, 1, 0>, ...)
- ...
Attachment #551145 - Attachment is obsolete: true
Attachment #552189 - Flags: review?(dvander)
Attached patch Cleanup (obsolete) — Splinter Review
Some minor cleanups
Attachment #552192 - Flags: review?(dvander)
Comment on attachment 552189 [details] [diff] [review]
implement JSOP_BITNOT

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

Sorry, I didn't think of this before, but I think the unary abstraction does not buy us anything here. There's only one unary bitop so we might as well just call everything BitOp instead of UnaryBitOp.

::: js/src/ion/IonBuilder.cpp
@@ +365,5 @@
>        case JSOP_IFEQ:
>          return jsop_ifeq(JSOP_IFEQ);
>  
> +      case JSOP_BITNOT:
> +        return jsop_unarybitop(op);

jsop_bitnot

@@ +1465,5 @@
>      return true;
>  }
>  
>  bool
> +IonBuilder::jsop_unarybitop(JSOp op)

jsop_bitnot, and remove op input

::: js/src/ion/LIR-Common.h
@@ +247,2 @@
>  // a 32-bit integer result as an output.
> +class LUnaryBitOp : public LInstructionHelper<1, 1, 0>

Rename to LBitNot.

::: js/src/ion/LOpcodes.h
@@ +50,5 @@
>      _(Value)                        \
>      _(Parameter)                    \
>      _(TableSwitch)                  \
>      _(Goto)                         \
> +    _(UnaryBitOp)                   \

Ditto.

::: js/src/ion/Lowering.cpp
@@ +161,5 @@
>      }
>  }
>  
>  bool
> +LIRGenerator::lowerUnaryBitOp(JSOp op, MInstruction *ins)

Rename to lowerBitNot, remove op input, use MBitNot.

::: js/src/ion/MIR.h
@@ +942,5 @@
> +
> +    void infer(const TypeOracle::Unary &u);
> +};
> +
> +class MBitNot : public MUnaryBitwiseInstruction

Fold MUnaryBitwiseInstruction into MBitNot.
Attachment #552189 - Flags: review?(dvander)
Now without the unary/binary layer
Attachment #552189 - Attachment is obsolete: true
Attachment #552209 - Flags: review?(dvander)
Attached patch Cleanup (obsolete) — Splinter Review
Attachment #552192 - Attachment is obsolete: true
Attachment #552192 - Flags: review?(dvander)
Attachment #552211 - Flags: review?(dvander)
Attached patch CleanupSplinter Review
Sry for the spam, but just saw I forget to "qrefresh" before uploading the cleanup patch.
Attachment #552211 - Attachment is obsolete: true
Attachment #552211 - Flags: review?(dvander)
Attachment #552225 - Flags: review?(dvander)
Attachment #552209 - Flags: review?(dvander) → review+
Comment on attachment 552225 [details] [diff] [review]
Cleanup

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

::: js/src/ion/Lowering.cpp
@@ +172,3 @@
>  
> +    ReorderCommutative(&lhs, &rhs);
> +    return lowerForALU(new LBitOp(op), ins, lhs, rhs);

Eventually we will have LBitOpIC or something which can handle arbitrary values on either side - so let's keep the old version of this function for now. (If the assert is firing, we should file a bug.)
Attachment #552225 - Flags: review?(dvander) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: