IonMonkey: implement BITNOT

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Implement JSOP_BITNOT in ionmonkey.
(Assignee)

Comment 1

7 years ago
Created attachment 551145 [details] [diff] [review]
implement JSOP_BITNOT
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+
(Assignee)

Comment 3

7 years ago
Created attachment 552189 [details] [diff] [review]
implement JSOP_BITNOT

- 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)
(Assignee)

Comment 4

7 years ago
Created attachment 552192 [details] [diff] [review]
Cleanup

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)
(Assignee)

Comment 6

7 years ago
Created attachment 552209 [details] [diff] [review]
implement JSOP_BITNOT

Now without the unary/binary layer
Attachment #552189 - Attachment is obsolete: true
Attachment #552209 - Flags: review?(dvander)
(Assignee)

Comment 7

7 years ago
Created attachment 552211 [details] [diff] [review]
Cleanup
Attachment #552192 - Attachment is obsolete: true
Attachment #552192 - Flags: review?(dvander)
Attachment #552211 - Flags: review?(dvander)
(Assignee)

Comment 8

7 years ago
Created attachment 552225 [details] [diff] [review]
Cleanup

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+
(Assignee)

Comment 10

7 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/29bd198d1ad1
http://hg.mozilla.org/projects/ionmonkey/rev/3a68a95b215b
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.