Bug 122205

Summary: FTL: Refactor compileArithDiv and compileArithMod into one function.
Product: WebKit Reporter: Nadav Rotem <nrotem>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Nadav Rotem 2013-10-01 22:54:45 PDT
FTL: Refactor compileArithDiv and compileArithMod into one function.
Comment 1 Nadav Rotem 2013-10-01 22:55:10 PDT
Created attachment 213153 [details]
Patch
Comment 2 Darin Adler 2013-10-02 09:14:33 PDT
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?
Comment 3 Nadav Rotem 2013-10-02 09:30:29 PDT
Darin, I will split the enums and resubmit.
Comment 4 Nadav Rotem 2013-10-02 09:33:57 PDT
Created attachment 213169 [details]
Patch
Comment 5 Darin Adler 2013-10-02 09:36:13 PDT
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.
Comment 6 Nadav Rotem 2013-10-02 10:03:15 PDT
Created attachment 213172 [details]
Patch
Comment 7 Filip Pizlo 2013-10-02 10:24:31 PDT
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()?
Comment 8 Nadav Rotem 2013-10-02 10:52:09 PDT
Created attachment 213175 [details]
Patch
Comment 9 Nadav Rotem 2013-10-02 11:34:38 PDT
Filip and Darin, thanks for the code review!
Comment 10 WebKit Commit Bot 2013-10-02 11:51:02 PDT
Comment on attachment 213175 [details]
Patch

Clearing flags on attachment: 213175

Committed r156784: <http://trac.webkit.org/changeset/156784>
Comment 11 WebKit Commit Bot 2013-10-02 11:51:04 PDT
All reviewed patches have been landed.  Closing bug.