...
Created attachment 434688 [details] Patch
Comment on attachment 434688 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434688&action=review > Source/JavaScriptCore/ChangeLog:3 > + Add new patternes to instruction selector to utilize AND/EOR/ORR-with-shift and UMULL supported by ARM64 nit: typo: /patternes/patterns/. Can you fix the bugzilla title as well?
Created attachment 434698 [details] Patch
Created attachment 434713 [details] Patch
Comment on attachment 434713 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434713&action=review New bitop+shift selection looks good. One suggestion there. But I'm confused by changes in call sites of isMergeableValue > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2675 > + if (isMergeableValue(multiplyLeft, SExt32) && isMergeableValue(multiplyRight, SExt32)) How is this behavior change sound? We're no longer checking canBeInternal, but we're still calling commitInternal. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2678 > + if (isMergeableValue(multiplyLeft, ZExt32) && isMergeableValue(multiplyRight, ZExt32)) ditto > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2753 > + if (isMergeableValue(multiplyLeft, SExt32) && isMergeableValue(multiplyRight, SExt32)) ditto > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2876 > + commitInternal(left); > + commitInternal(right); ditto regarding not checking canBeInternal. > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2998 > + // and-with-shift Pattern: n & (m ShiftType amount) nit: I'd use the names the function uses in this comment: "and-with-shift Pattern: left & (right ShiftType amount)" > Source/JavaScriptCore/b3/B3LowerToAir.cpp:3284 > + auto tryAppendXorWithShift = [&] (Value* left, Value* right) -> bool { tryAppendXorWithShift and tryAppendAndWithShift and tryAppendOrWithShift are all identical minus the opcodes passed to the opcodeBasedOnShiftKind call. Can we abstract these 3 into a function shared by all? Maybe tryAppendBitOpWithShift or something? You can still keep all 3 lambdas, just so it's easier to call with (left, right) and (right, left), but the lambdas can call a shared helper.
Created attachment 434795 [details] Patch
Created attachment 434806 [details] Patch
Comment on attachment 434806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434806&action=review Nice. r=me > Source/JavaScriptCore/b3/B3LowerToAir.cpp:2550 > + auto tryAppendBitOpWithShift = [&] (Value* left, Value* right, Air::Opcode opcode) -> bool { > + if (!isValidForm(opcode, Arg::Tmp, Arg::Tmp, Arg::Imm, Arg::Tmp)) > + return false; > + if (!canBeInternal(right) || !imm(right->child(1)) || right->child(1)->asInt() < 0) > + return false; > + > + uint64_t amount = right->child(1)->asInt(); > + uint64_t datasize = m_value->type() == Int32 ? 32 : 64; > + if (amount >= datasize) > + return false; > + > + append(opcode, tmp(left), tmp(right->child(0)), imm(right->child(1)), tmp(m_value)); > + commitInternal(right); > + return true; > + }; I think the way we normally do this is just by making it a member function.
Created attachment 434809 [details] Patch for landing
Created attachment 434810 [details] Patch for landing
Committed r280579 (240201@main): <https://commits.webkit.org/240201@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 434810 [details].
<rdar://problem/81441719>