Bug 152810

Created attachment 268416 [details] work in progress

Created attachment 268439 [details] it just might work

Created attachment 268441 [details] even nicer Of course, I haven't tried compiling it yet.

Created attachment 268444 [details] it did things It's starting to work.

Created attachment 268471 [details] seems to work

Running tests now.

Created attachment 268474 [details] the patch

Attachment 268474 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:52: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:58: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:67: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:68: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:69: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:88: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:91: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:104: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:105: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:106: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:107: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.cpp:108: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLB3Compile.cpp:123: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:357: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:360: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 15 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 268474 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review > Source/JavaScriptCore/dfg/DFGCommon.h:110 > - // We're converging in a straight-forward forward flow fixpoint. This is the > + // We're converging in a straght-forward forward flow fixpoint. This is the Revert.

(In reply to comment #9) > Comment on attachment 268474 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268474&action=review > > > Source/JavaScriptCore/dfg/DFGCommon.h:110 > > - // We're converging in a straight-forward forward flow fixpoint. This is the > > + // We're converging in a straght-forward forward flow fixpoint. This is the > > Revert. Lol done.

Comment on attachment 268474 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review > Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49 > + // At any time, you can I'll revert this.

Comment on attachment 268474 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=268474&action=review r=me with some style comments. > Source/JavaScriptCore/dfg/DFGCommon.h:110 > + // We're converging in a straght-forward forward flow fixpoint. This is the typo > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:46 > +Box<CCallHelpers::JumpList> ExceptionTarget::jumps(CCallHelpers& jit) nice. > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:70 > +{ you could assert to verify that defaultHandler is a real thing only when isDefaultHandler is true to prevent miscreation. > Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49 > + // At any time, you can remove > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:355 > + auto exceptionHandler = state->exceptionHandler; style: I think this is much easier to read and to determine that it's correct if you just used "Box<Label>" type here instead of auto. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7226 > + // FIXME: If this is a GetByIdFlush, we might get some performance boost if we claim that it > + // clobbers volatile registers late. It's not necessary for correctness, though, since the > + // IC code is super smart about saving registers. might be worth opening a bug. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7242 > + // This is the direct exit target for operation calls. It's a pointer to a JumpList. > + auto exceptions = exceptionHandle->scheduleExitCreation(params)->jumps(jit); You could just remove this comment if you removed auto and wrote out the type, and also just named it something like "exceptionTargetForCallOperation". I think that's more readable than having a comment. > Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.h:85 > + RefPtr<ExceptionTarget> scheduleExitCreation(const B3::StackmapGenerationParams&); > + > + // Schedules the creation of an OSR exit jump destination, and ensures that it gets associated > + // with the handler for some callsite index. > + void scheduleExitCreationForUnwind(const B3::StackmapGenerationParams&, CallSiteIndex); nice.

(In reply to comment #12) > Comment on attachment 268474 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268474&action=review > > r=me with some style comments. > > > Source/JavaScriptCore/dfg/DFGCommon.h:110 > > + // We're converging in a straght-forward forward flow fixpoint. This is the > > typo Done. > > > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:46 > > +Box<CCallHelpers::JumpList> ExceptionTarget::jumps(CCallHelpers& jit) > > nice. > > > Source/JavaScriptCore/ftl/FTLExceptionTarget.cpp:70 > > +{ > > you could assert to verify that defaultHandler is a real thing only when > isDefaultHandler is true to prevent miscreation. Meh. It's already a private constructor friended to PatchpointExceptionHandle. > > > Source/JavaScriptCore/ftl/FTLExceptionTarget.h:49 > > + // At any time, you can > > remove Done. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:355 > > + auto exceptionHandler = state->exceptionHandler; > > style: I think this is much easier to read and to determine that it's > correct if you just used "Box<Label>" type here instead of auto. Done. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7226 > > + // FIXME: If this is a GetByIdFlush, we might get some performance boost if we claim that it > > + // clobbers volatile registers late. It's not necessary for correctness, though, since the > > + // IC code is super smart about saving registers. > > might be worth opening a bug. Done. > > > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:7242 > > + // This is the direct exit target for operation calls. It's a pointer to a JumpList. > > + auto exceptions = exceptionHandle->scheduleExitCreation(params)->jumps(jit); > > You could just remove this comment if you removed auto and wrote out the > type I'm fine with this. > , and also just named it something like > "exceptionTargetForCallOperation". > I think that's more readable than having a comment. I prefer the "This is the direct exit target for operation calls" part as a comment. I've been having a thing for shorter variable names recently. I think that it works well, in this case, after I write out the type. The code is now: // This is the direct exit target for operation calls. Box<CCallHelpers::JumpList> exceptions = exceptionHandle->scheduleExitCreation(params)->jumps(jit); > > > Source/JavaScriptCore/ftl/FTLPatchpointExceptionHandle.h:85 > > + RefPtr<ExceptionTarget> scheduleExitCreation(const B3::StackmapGenerationParams&); > > + > > + // Schedules the creation of an OSR exit jump destination, and ensures that it gets associated > > + // with the handler for some callsite index. > > + void scheduleExitCreationForUnwind(const B3::StackmapGenerationParams&, CallSiteIndex); > > nice.

Landed in http://trac.webkit.org/changeset/194716