Bug 122205 – FTL: Refactor compileArithDiv and compileArithMod into one function.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nadav Rotem]

FTL: Refactor compileArithDiv and compileArithMod into one function.

[Nadav Rotem]

Created attachment 213153 [details]
Patch

[Darin Adler]

Comment on attachment 213153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213153&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:657
> -    enum AddOrSubKind {Add, Sub};
> -    void compileAddSub(AddOrSubKind opKind)
> +    enum ArithKind {Add, Sub, Div, Mod};

Doesn’t seem helpful to share a single enum for these four kinds of arithmetic. Why not a second enum?

[Nadav Rotem]

Darin, I will split the enums and resubmit.

[Nadav Rotem]

Created attachment 213169 [details]
Patch

[Darin Adler]

Comment on attachment 213169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213169&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:783
> +    enum DivOrModKind {Div, Mod};

Formatting error. We use spaces inside the braces.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:787
> +        bool isDiv = opKind == Div;

Seems like we could say "opKind == Div" at the two sites where this is used. The boolean doesn’t add significant clarity.

[Nadav Rotem]

Created attachment 213172 [details]
Patch

[Filip Pizlo]

Comment on attachment 213172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213172&action=review

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:294
> -            compileArithDiv();
> +            compileArithDivMod(Div);
>              break;
>          case ArithMod:
> -            compileArithMod();
> +            compileArithDivMod(Mod);

General comment: is there a reason why we even have an enum that we pass to these?

For example, compileArithMinOrMax() doesn't take any arguments; it figures out what it should do by looking at m_node->op() - either it's ArithMin or ArithMax.  Hence no need for an argument or a separate enum.

What do you think about that for DivOrMod() and AddOrSub()?

[Nadav Rotem]

Created attachment 213175 [details]
Patch

[Nadav Rotem]

Filip and Darin, thanks for the code review!

[WebKit Commit Bot]

Comment on attachment 213175 [details]
Patch

Clearing flags on attachment: 213175

Committed r156784: <http://trac.webkit.org/changeset/156784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.