Bug 151114

Created attachment 265215 [details] work in progress

Created attachment 265223 [details] more

Created attachment 265247 [details] the patch

Attachment 265247 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3BlockInsertionSet.h:78: The parameter name "block" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:45: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:46: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:47: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 47 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 265251 [details] the patch With some fixes.

Created attachment 265255 [details] the patch Fix 32-bit.

Comment on attachment 265255 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=265255&action=review > Source/JavaScriptCore/ChangeLog:22 > + This introduces an idiom that handles this. It's BlockInsertionSet::splitBefore(). The > + idea is that it uses the current block before the split as the continuation after the > + split. When you call splitBefore(), you pass it your loop index and your InsertionSet > + (if applicable). It makes sure that it changes those auxiliary things in such a way that > + you can keep looping. > + splitBefore() -> splitForward() > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:394 > + void x86ConvertToDoubleWord64(RegisterID rax, RegisterID rdx) ConvertToQuadword? > Source/JavaScriptCore/assembler/X86Assembler.h:1295 > + void cdqq() Looks like the official name for this is "cqo" > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:75 > +void updatePredecessors(BasicBlock* root) updatePredecessors -> updatePredecessorsAfter(block)? The name is a bit weird. > Source/JavaScriptCore/b3/B3Common.h:101 > + if (den == -1 && num == -2147483647 - 1) Why not use numeric_limits? > Source/JavaScriptCore/b3/B3Common.h:110 > + if (den == -1 && num == -9223372036854775807ll - 1) ditto, and some later. > Source/JavaScriptCore/b3/B3LowerMacros.cpp:59 > + m_changed |= m_blockInsertionSet.execute(); Having this at the end means no Macro can use a Macro in its expansion. I guess you could assert that by running the algorithm again in Debug and checking that m_blockInsertionSet.execute() returns false. > Source/JavaScriptCore/b3/B3LowerMacros.cpp:90 > + // res = num / dev; dev -> den > Source/JavaScriptCore/b3/B3LowerMacros.cpp:97 > + Value* one = > + m_insertionSet.insertIntConstant(m_index, m_value, 1); Could be on a single line.

Comment on attachment 265255 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=265255&action=review >> Source/JavaScriptCore/ChangeLog:22 >> + > > splitBefore() -> splitForward() Or maybe just split()? (The split is forward and backward, and it breaks what comes before from what comes after.)

(In reply to comment #7) > Comment on attachment 265255 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265255&action=review > > > Source/JavaScriptCore/ChangeLog:22 > > + This introduces an idiom that handles this. It's BlockInsertionSet::splitBefore(). The > > + idea is that it uses the current block before the split as the continuation after the > > + split. When you call splitBefore(), you pass it your loop index and your InsertionSet > > + (if applicable). It makes sure that it changes those auxiliary things in such a way that > > + you can keep looping. > > + > > splitBefore() -> splitForward() > > > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:394 > > + void x86ConvertToDoubleWord64(RegisterID rax, RegisterID rdx) > > ConvertToQuadword? Good point. > > > Source/JavaScriptCore/assembler/X86Assembler.h:1295 > > + void cdqq() > > Looks like the official name for this is "cqo" > > > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:75 > > +void updatePredecessors(BasicBlock* root) > > updatePredecessors -> updatePredecessorsAfter(block)? > The name is a bit weird. Sure. > > > Source/JavaScriptCore/b3/B3Common.h:101 > > + if (den == -1 && num == -2147483647 - 1) > > Why not use numeric_limits? I guess I should! This was old school paranoia about -2147483648 not being a valid constant. Seems that numeric_limits does the right thing though. > > > Source/JavaScriptCore/b3/B3Common.h:110 > > + if (den == -1 && num == -9223372036854775807ll - 1) > > ditto, and some later. > > > Source/JavaScriptCore/b3/B3LowerMacros.cpp:59 > > + m_changed |= m_blockInsertionSet.execute(); > > Having this at the end means no Macro can use a Macro in its expansion. That's true! I think it would be bad form to have an expansion that requires more expansions. Good discipline about this prevents us from having to worry about fixpointing this phase, or having the phase never converge. > > I guess you could assert that by running the algorithm again in Debug and > checking that m_blockInsertionSet.execute() returns false. We could. The phase also returns a bool saying if it changed anything. We could internally have the phase run itself twice. > > > Source/JavaScriptCore/b3/B3LowerMacros.cpp:90 > > + // res = num / dev; > > dev -> den > > > Source/JavaScriptCore/b3/B3LowerMacros.cpp:97 > > + Value* one = > > + m_insertionSet.insertIntConstant(m_index, m_value, 1); > > Could be on a single line.

(In reply to comment #8) > Comment on attachment 265255 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265255&action=review > > >> Source/JavaScriptCore/ChangeLog:22 > >> + > > > > splitBefore() -> splitForward() > > Or maybe just split()? (The split is forward and backward, and it breaks > what comes before from what comes after.) Well, if we wanted to split blocks when transforming in a backward algorithm, then we'd want the newly created block to hold things *above* the valueIndex. Hence why I call it "splitForward()". The "Forward" tells you that it's meant to be used in a transformation that walks forward.

Created attachment 265269 [details] patch for landing

Attachment 265269 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:47: The parameter name "proc" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/b3/testb3.cpp:209: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:302: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:2855: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/testb3.cpp:2861: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 5 in 17 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 265270 [details] patch for landing The right patch this time!

Created attachment 265271 [details] patch for landing

Comment on attachment 265271 [details] patch for landing Clearing flags on attachment: 265271 Committed r192295: <http://trac.webkit.org/changeset/192295>

All reviewed patches have been landed. Closing bug.