[JIT] X64/ARM64 - `CAST` removals for small types on `ADD`, `SUB`, `MUL`, `AND`, `OR`, `XOR`, `NOT`, `NEG` #77874

TIHan · 2022-11-04T02:38:03Z

Description

Should address most of: #44849

The transformation here is straight-forward, consider the example IR:

CAST(ADD(CAST(x), CAST(y)))

can be safely transformed in lowering into:

CAST(ADD(x, y))

This removes the CAST for the inputs on the operations ADD, SUB, MUL, AND, OR, XOR, NOT, and NEG. Removing the CAST gets rid of movzx / movsx instructions on X64, and uxtb / sxtb instructions on ARM64. This is safe to do because it doesn't matter if the inputs are zero/sign-extended as the result of the operations are zero/signed-extended anyway and the operations of the upper bits do not affect the lower bits.

Acceptance Criteria

Disasm tests for Add, Sub, Mul, And, Or
Regression tests
Merge [JIT] X64 - Allow containment of larger memory operands for AND, OR, XOR #79126

ghost · 2022-11-04T02:38:16Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	TIHan
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

…nd neg/not.

TIHan · 2022-11-05T03:04:19Z

/azp run Fuzzlyn

azure-pipelines · 2022-11-05T03:04:34Z

Azure Pipelines successfully started running 1 pipeline(s).

…dd-opt-x64

…ation if the op is a local

…ly on the CastOp.

TIHan · 2022-11-10T02:18:15Z

/azp run Fuzzlyn

azure-pipelines · 2022-11-10T02:18:30Z

Azure Pipelines successfully started running 1 pipeline(s).

TIHan · 2022-11-10T17:35:58Z

There is a minor issue that we think is not caused by this caused, but the change is allowing the issue to manifest itself. Here is a dump of some potential wrong IR for value number:

***** BB04
STMT00003 ( 0x059[E-] ... 0x05D )
N005 (  4,  4)              [000014] -A------R--                         *  ASG       short  $VN.Void
N004 (  2,  2)              [000013] D------N---                         +--*  LCL_VAR   short  V00 arg0         d:4 $VN.Void
N003 (  4,  4) CSE #01 (def)[000011] -----------                         \--*  ADD       int    <l:$41, c:$3c5>
N001 (  2,  2)              [000009] -----------                            +--*  LCL_VAR   int    V00 arg0         u:3 (last use) <l:$46, c:$3c4>
N002 (  1,  1)              [000010] -----------                            \--*  CNS_INT   int    1 $43

***** BB04
STMT00004 ( 0x05F[E-] ... 0x060 )
N003 (  4,  5)              [000016] -----------                         *  RETURN    int    $VN.Void
N002 (  3,  4) CSE #01 (use)[000172] -----------                         \--*  CAST      int <- short <- int <l:$41, c:$3c7>
N001 (  2,  2)              [000015] -----------                            \--*  LCL_VAR   int    V00 arg0         u:4 (last use) <l:$41, c:$3c6>

JulieLeeMSFT · 2023-01-13T17:36:32Z

CC @markples for code review.

BruceForstall requested a review from [markples].

markples · 2023-01-13T20:25:59Z

[edit - added one more to keep them together]

General comments on the tests:

👍 extensive coverage
You use three styles for expressing the answer to some operations:
```
if (Test1() != 123) ...
if (Test2() != 234) ...
```
```
var result1 = Test1();
var result2 = Test2();
...
if (result1 != 123) ...
if (result2 != 234) ...
```
```
if (!Test1()) ... // internally checks 123
if (!Test2()) ... // internally checks 234
```
The last is the most readable (the entire individual test is in one spot), then the first, and the second is the most difficult. Though thinking about this, [Fact]/[Theory] are probably the best way to go as you don't even need a Main anymore (and then you get automatically reporting of all tests rather than stopping at the first failure. I've been meaning to mess around with this in our tree, so if you want I could attempt to convert this PR.
What does the use of "Regression" in the test filenames indicate?
How did you verify that you have the right answers for the various tests? And similar for the FileCheck lines? I'm trying to figure out the correct level of review for them.

src/tests/JIT/opt/And/IntAnd.cs

markples · 2023-01-13T20:31:11Z

src/tests/JIT/opt/And/IntAnd.cs

+
+            if ((byte)(x & y) == 0)
+            {
+                SideEffect();


What is SideEffect doing here? Do we get different code generation if we are only producing 1/0 vs needing control flow?

I need to have some sort of side effect to happen. Because if I do this example:

if ((byte)(x & y) == 0) { return true; } return false;

I'm not going to get a branch.

I need a branch in order to see the and op using 16-bit or 8-bit operations. We only use 16-bit/8-bit operations for 'and','or','xor' if it involves a branch.

markples · 2023-01-13T20:32:42Z

src/tests/JIT/opt/And/IntAnd.cs

+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static bool Test_UInt32_UInt32_CastByte_And(uint x, uint y)


Do you need a CastByte variant for the RHS of the operation to make sure that both tree shapes work?

Not sure what you mean here. What is the RHS in this case?

if ((byte)(x & (byte)y) == 0)

Sorry, RHS was inaccurate.. "right operand of the &" would have been better

I see, I think it's fine and we don't need to add another test case. In this test case, if the second op of and is an 8-bit register, I can feel pretty confident that casting the byte will still result in using an 8-bit register.

The next test case below this one casts both ops and still gives an 8-bit register.

src/tests/JIT/opt/Divide/Regressions/Regression1.cs

src/tests/JIT/opt/And/IntAnd.cs

markples · 2023-01-13T20:44:59Z

src/tests/JIT/opt/Regressions/Regression1.cs

+   public static sbyte[] M75()
+   {
+       return default(sbyte[]);
+   }
+}
+
+public interface IRuntime
+{
+   void WriteLine<T>(T value);
+}
+
+public class Runtime : IRuntime
+{
+   public void WriteLine<T>(T value) { }
+}


I don't really follow what this test is doing. There's a bit of short arithmetic - do the try, M75 call, WriteLines, etc., all contribute to some codegen impact?

I'll have similar questions for the other tests like this, so I'll wait to hear what this is before trying to understand them.

I see your comment above about the test being brittle (failing to repro with changes). It seems like it is likely that a future JIT change will impact the IR so that this test won't be useful anymore. I'm not sure what to do about that, though if there's nothing productive to do, perhaps having the conditions in the JIT that led to incorrect behavior listed in the test would be helpful?

This test was generated by Fuzzlyn that caught an assertion/wrong value in the early implementation of this change. So I kept this test to make sure that it doesn't happen again. It's not checking for any disassembly, it is just to make sure it gives the correct result.

src/tests/JIT/opt/Regressions/Regression2.cs

TIHan · 2023-01-13T22:26:52Z

Thank you for the feedback @markples

TIHan · 2023-01-13T23:18:05Z

You use three styles for expressing the answer to some operations:

Because these are tests, I'm not committed to only use a certain style. If the test does what it is supposed to do and looks readable enough, then it's fine to me.

What does the use of "Regression" in the test filenames indicate?

It's as it sounds, it is just a Regression test. I wasn't sure what to name it because we don't have a corresponding issue for this kind of test on GitHub. So the name, Regression, in this case is to simply for having regression test cases that do not correspond to a specific issue.

How did you verify that you have the right answers for the various tests? And similar for the FileCheck lines? I'm trying to figure out the correct level of review for them.

For the right answers, I run the test in different versions of .NET to see if I get the same results - if I do, then it's very highly likely that it's correct. I feel confident about this because in the early implementation of this change, some of those tests started failing, so I know that it is covering it.

For FileCheck lines, I use @EgorBo 's awesome tool Disasmo to look at the disassembly of each test case in Visual Studio; I also check it with main branch of the runtime to verify the assembly was different before.

TIHan · 2023-01-17T21:51:03Z

@EgorBo PTAL - the CI failures are unrelated and here are the diffs.

…dd-opt-x64

TIHan · 2023-01-30T18:25:19Z

Hey @EgorBo , just pinging again. I fixed the merge conflicts and turned off the optimization if optimizations are disabled; I figure that is the right call.

Trying to optimize Add

f6bb7d4

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 4, 2022

ghost assigned TIHan Nov 4, 2022

TIHan added 4 commits November 3, 2022 19:50

Trying to optimize Add

81a1bcb

Trying to optimize Add

1b29b47

Merge remote-tracking branch 'upstream/main' into add-opt-x64

2599997

Added IntAdd disasm tests. Expanded cast removals to all arithmetic a…

5e3741c

…nd neg/not.

TIHan changed the title ~~Trying to optimize Add~~ [JIT] X64 - Minor optimizations for arithmetic and not / neg Nov 5, 2022

Merge branch 'main' into add-opt-x64

371ca7d

TIHan added 9 commits November 4, 2022 22:46

Checked unsigned

2a43021

Limit arithmetic opt

5428df9

Merge branch 'add-opt-x64' of https://github.com/TIHan/runtime into a…

ac58639

…dd-opt-x64

Merge remote-tracking branch 'upstream/main' into add-opt-x64

7badcc0

Moving optimization to Morph

6d85c1a

Cleanup. Allow more opts.

02dbb22

Fix build

779dd21

More opts

a263faa

Added regression test. Allowing all arithmetic ops. Only do transform…

f7c8b68

…ation if the op is a local

build-analysis bot mentioned this pull request Nov 9, 2022

Tracking issue for CI build timeouts #76454

Closed

TIHan added 3 commits November 9, 2022 02:21

Do not do optimization for unsigned ops

3eba2fa

Added more division test cases

ec3dcda

Fixed test. Checking overflows on cast. Comparing actual types proper…

183d570

…ly on the CastOp.

Added flag

4378c1b

Remove flag

4a0b63d

markples reviewed Jan 13, 2023

View reviewed changes

src/tests/JIT/opt/And/IntAnd.cs Show resolved Hide resolved

markples reviewed Jan 13, 2023

View reviewed changes

src/tests/JIT/opt/Divide/Regressions/Regression1.cs Outdated Show resolved Hide resolved

markples reviewed Jan 13, 2023

View reviewed changes

src/tests/JIT/opt/And/IntAnd.cs Outdated Show resolved Hide resolved

markples reviewed Jan 13, 2023

View reviewed changes

src/tests/JIT/opt/Regressions/Regression2.cs Outdated Show resolved Hide resolved

TIHan added 5 commits January 13, 2023 14:29

Removed unnecessary code.

23427f6

Update IntAnd.cs

520be33

Update Regression1.cs

7cac5e2

Update Regression1.cs

c6a31fe

Update Regression2.cs

2fbe829

Update Regression2.cs

3384759

TIHan closed this Jan 26, 2023

TIHan reopened this Jan 26, 2023

TIHan added 4 commits January 30, 2023 10:10

Merge remote-tracking branch 'upstream/main' into add-opt-x64

2ea70d4

Disable if optimizations are disabled

40b59c2

Merge branch 'add-opt-x64' of https://github.com/TIHan/runtime into a…

051d513

…dd-opt-x64

Merge

25784fb

EgorBo approved these changes Jan 31, 2023

View reviewed changes

TIHan merged commit 8553375 into dotnet:main Jan 31, 2023

TIHan mentioned this pull request Jan 31, 2023

Optimize arithmetic/bitwise operations of short, ushort, sbyte, and byte. #44849

Closed

JulieLeeMSFT mentioned this pull request Feb 8, 2023

What's new in .NET 8 Preview 1 dotnet/core#8133

Closed

3 tasks

ghost locked as resolved and limited conversation to collaborators Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JIT] X64/ARM64 - `CAST` removals for small types on `ADD`, `SUB`, `MUL`, `AND`, `OR`, `XOR`, `NOT`, `NEG` #77874

[JIT] X64/ARM64 - `CAST` removals for small types on `ADD`, `SUB`, `MUL`, `AND`, `OR`, `XOR`, `NOT`, `NEG` #77874

TIHan commented Nov 4, 2022 •

edited

Loading

ghost commented Nov 4, 2022

TIHan commented Nov 5, 2022

azure-pipelines bot commented Nov 5, 2022

TIHan commented Nov 10, 2022

azure-pipelines bot commented Nov 10, 2022

TIHan commented Nov 10, 2022

JulieLeeMSFT commented Jan 13, 2023

markples commented Jan 13, 2023 •

edited

Loading

markples Jan 13, 2023

TIHan Jan 13, 2023 •

edited

Loading

markples Jan 13, 2023

TIHan Jan 13, 2023

markples Jan 13, 2023

TIHan Jan 13, 2023

TIHan Jan 13, 2023

markples Jan 13, 2023

markples Jan 13, 2023

TIHan Jan 13, 2023

TIHan commented Jan 13, 2023

TIHan commented Jan 13, 2023

TIHan commented Jan 17, 2023

TIHan commented Jan 30, 2023

[JIT] X64/ARM64 - CAST removals for small types on ADD, SUB, MUL, AND, OR, XOR, NOT, NEG #77874

[JIT] X64/ARM64 - CAST removals for small types on ADD, SUB, MUL, AND, OR, XOR, NOT, NEG #77874

Conversation

TIHan commented Nov 4, 2022 • edited Loading

ghost commented Nov 4, 2022

TIHan commented Nov 5, 2022

azure-pipelines bot commented Nov 5, 2022

TIHan commented Nov 10, 2022

azure-pipelines bot commented Nov 10, 2022

TIHan commented Nov 10, 2022

JulieLeeMSFT commented Jan 13, 2023

markples commented Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

TIHan Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan commented Jan 13, 2023

TIHan commented Jan 13, 2023

TIHan commented Jan 17, 2023

TIHan commented Jan 30, 2023

[JIT] X64/ARM64 - `CAST` removals for small types on `ADD`, `SUB`, `MUL`, `AND`, `OR`, `XOR`, `NOT`, `NEG` #77874

[JIT] X64/ARM64 - `CAST` removals for small types on `ADD`, `SUB`, `MUL`, `AND`, `OR`, `XOR`, `NOT`, `NEG` #77874

TIHan commented Nov 4, 2022 •

edited

Loading

markples commented Jan 13, 2023 •

edited

Loading

TIHan Jan 13, 2023 •

edited

Loading