Bug 226937 - Add Air opcode sub32/64(Reg, Imm, Reg) form for ARM64 and select this instruction in Air
Summary: Add Air opcode sub32/64(Reg, Imm, Reg) form for ARM64 and select this instruc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords:
Depends on: 227060
Blocks:
  Show dependency treegraph
 
Reported: 2021-06-11 12:07 PDT by Yijia Huang
Modified: 2021-06-16 12:14 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.82 KB, patch)
2021-06-13 14:28 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.84 KB, patch)
2021-06-13 14:36 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (15.06 KB, patch)
2021-06-13 20:42 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (15.03 KB, patch)
2021-06-13 20:47 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.66 KB, patch)
2021-06-14 08:40 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2021-06-14 08:44 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2021-06-16 11:25 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-06-11 12:07:27 PDT
...
Comment 1 Yijia Huang 2021-06-13 14:28:50 PDT
Created attachment 431287 [details]
Patch
Comment 2 Yijia Huang 2021-06-13 14:36:08 PDT
Created attachment 431288 [details]
Patch
Comment 3 Yijia Huang 2021-06-13 20:42:26 PDT
Created attachment 431299 [details]
Patch
Comment 4 Yijia Huang 2021-06-13 20:47:32 PDT
Created attachment 431300 [details]
Patch
Comment 5 Filip Pizlo 2021-06-14 08:26:05 PDT
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.
Comment 6 Yijia Huang 2021-06-14 08:40:53 PDT
Created attachment 431326 [details]
Patch
Comment 7 Yijia Huang 2021-06-14 08:44:11 PDT
Created attachment 431328 [details]
Patch
Comment 8 Saam Barati 2021-06-14 10:13:11 PDT
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 9 Saam Barati 2021-06-14 10:14:13 PDT
Comment on attachment 431328 [details]
Patch

Everything else looks really good. Your testmasm tests are good
Comment 10 Saam Barati 2021-06-14 10:18:44 PDT
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
Comment 11 Yijia Huang 2021-06-14 10:21:33 PDT
(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.
Comment 12 EWS 2021-06-14 12:44:08 PDT
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].
Comment 13 WebKit Commit Bot 2021-06-16 02:37:21 PDT
Re-opened since this is blocked by bug 227060
Comment 14 Yijia Huang 2021-06-16 11:25:20 PDT
Created attachment 431572 [details]
Patch
Comment 15 Yusuke Suzuki 2021-06-16 11:29:07 PDT
Comment on attachment 431572 [details]
Patch

rs=me for relanding this patch since it was not the cause of the regression.
Comment 16 EWS 2021-06-16 12:14:33 PDT
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].