FTL: Refactor compileArithDiv and compileArithMod into one function.
Created attachment 213153 [details] Patch
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?
Darin, I will split the enums and resubmit.
Created attachment 213169 [details] Patch
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.
Created attachment 213172 [details] Patch
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()?
Created attachment 213175 [details] Patch
Filip and Darin, thanks for the code review!
Comment on attachment 213175 [details] Patch Clearing flags on attachment: 213175 Committed r156784: <http://trac.webkit.org/changeset/156784>
All reviewed patches have been landed. Closing bug.