...
Created attachment 431287 [details] Patch
Created attachment 431288 [details] Patch
Created attachment 431299 [details] Patch
Created attachment 431300 [details] Patch
Comment on attachment 431300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431300&action=review R- but only because part of your change removes something that we don't want to remove. > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:-636 > - // Turn this: Sub(value, constant) > - // Into this: Add(value, -constant) > - if (Value* negatedConstant = m_value->child(1)->negConstant(m_proc)) { > - m_insertionSet.insertValue(m_index, negatedConstant); > - replaceWithNew<Value>( > - Add, m_value->origin(), m_value->child(0), negatedConstant); > - break; > - } > - I don't think you want to remove this. I get that this strength reduction makes it harder to test your change. But, this strength reduction is necessary so that other things work: - It's important for strength reduction to bring programs into a canonical form. That is, to reduce the likelihood that two programs that do the same thing have different IR. Canonicalization makes it easier to then remove redundant code. For example: say that the program first did "Add(x, -42)" and then did "Sub(x, 42)". So, the two pieces of code do not seem redundant, so common subexpression elimination (CSE) won't remove the second one. This canonicalization rule that you removed makes it so they will both say "Add(x, -42)", so then CSE can remove the second one. – Address expressions on both x86 and arm64 are matched by looking for "Add(something, constant)". So, it's important for other parts of the instruction selector to see "Add(something, constant)" rather than "Sub(something, constant)". I would just revert this part of your change. If you want to make sure you're testing the instruction you added, you can write a testb3 test that skips reduceStrength. I'm not sure that's worth it, though. I would just land your change with this part reverted so you can move onto more interesting things.
Created attachment 431326 [details] Patch
Created attachment 431328 [details] Patch
Comment on attachment 431328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431328&action=review > Source/JavaScriptCore/b3/testb3_2.cpp:2095 > +void testSubArgs32ZeroExtend(int a, int b) I think we should adds tests for imm sub forms you added.
Comment on attachment 431328 [details] Patch Everything else looks really good. Your testmasm tests are good
Comment on attachment 431328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431328&action=review Reinstating r+ >> Source/JavaScriptCore/b3/testb3_2.cpp:2095 >> +void testSubArgs32ZeroExtend(int a, int b) > > I think we should adds tests for imm sub forms you added. Yijia informed me we already test this in testSubArgImm pre-existing test
(In reply to Saam Barati from comment #9) > Comment on attachment 431328 [details] > Patch > > Everything else looks really good. Your testmasm tests are good Actually, both 64-bit void testSubArgImm(int64_t a, int64_t b) and 32-bit void testSubArgImm32(int a, int b) are pre-existing in the testb3_2.cpp.
Committed r278846 (238792@main): <https://commits.webkit.org/238792@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431328 [details].
Re-opened since this is blocked by bug 227060
Created attachment 431572 [details] Patch
Comment on attachment 431572 [details] Patch rs=me for relanding this patch since it was not the cause of the regression.
Committed r278950 (238879@main): <https://commits.webkit.org/238879@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431572 [details].