Bug 226937 – Add Air opcode sub32/64(Reg, Imm, Reg) form for ARM64 and select this instruction in Air

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Yijia Huang]

Created attachment 431287 [details]
Patch

[Yijia Huang]

Created attachment 431288 [details]
Patch

[Yijia Huang]

Created attachment 431299 [details]
Patch

[Yijia Huang]

Created attachment 431300 [details]
Patch

[Filip Pizlo]

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.

[Yijia Huang]

Created attachment 431326 [details]
Patch

[Yijia Huang]

Created attachment 431328 [details]
Patch

[Saam Barati]

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.

[Saam Barati]

Comment on attachment 431328 [details]
Patch

Everything else looks really good. Your testmasm tests are good

[Saam Barati]

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

[Yijia Huang]

(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.

[EWS]

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].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227060

[Yijia Huang]

Created attachment 431572 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 431572 [details]
Patch

rs=me for relanding this patch since it was not the cause of the regression.

[EWS]

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].