Cleanup LOCKADD handling #18267

mikedn · 2018-06-03T21:51:05Z

LOCKADD nodes are generated rather early and there's no reason for that:

The CORINFO_INTRINSIC_InterlockedAdd32/64 intrinsics are not actually used. Even if they would be used we can still import them as XADD nodes and rely on lowering to generate LOCKADD when needed.
gtExtractSideEffList transforms XADD into LOCKADD but this can be done in lowering. LOCKADD is an XARCH specific optimization after all.

Additionally:

Avoid the need for special handling in LSRA by making LOCKADD a "no value" oper.
Split LOCKADD codegen from XADD/XCHG codegen, attempting to use the same code for all 3 just makes things more complex.
The address is always in a register so there's no real need to create an indir node on the fly, the relevant emitter functions can be called directly.

The last point above is actually a CQ issue - we always generate add [reg], imm, more complex address modes are not used. Unfortunately this problem starts early, when the importer spills the address to a local variable. If that ever gets fixed then we'll could probably generate a contained LEA in lowering.

Contributes to #14557

mikedn · 2018-06-04T15:57:10Z

@CarolEidt I've yet to convince myself that adding an indirection to interlocked nodes is a step forward. But even without that there's still room for improvement, especially around LOCKADD. There's no need to generate it early, it should be GTK_NOVALUE, there's no need to use the indirForm hack etc.

mikedn · 2018-06-04T17:02:12Z

jit-diff summary:

Total bytes of diff: -22 (0.00% of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file improvements by size (bytes):
         -15 : System.Private.CoreLib.dasm (0.00% of base)
          -3 : System.IO.Pipes.dasm (-0.01% of base)
          -2 : System.Linq.Parallel.dasm (0.00% of base)
          -1 : System.Collections.Concurrent.dasm (0.00% of base)
          -1 : System.Net.Security.dasm (0.00% of base)
5 total files with size differences (5 improved, 0 regressed), 125 unchanged.
Top method improvements by size (bytes):
          -6 : System.Private.CoreLib.dasm - MemoryFailPoint:.ctor(int):this
          -3 : System.IO.Pipes.dasm - PipeCompletionSource`1:RegisterForCancellation(struct):this (3 methods)
          -3 : System.Private.CoreLib.dasm - SpinLock:EnterSpin(int):this
          -3 : System.Private.CoreLib.dasm - MemoryFailPoint:Dispose(bool):this
          -1 : System.Collections.Concurrent.dasm - WorkStealingQueue:TryLocalPop(byref):bool:this
11 total methods with size differences (11 improved, 0 regressed), 145751 unchanged.

mikedn · 2018-06-04T17:19:23Z

There are 2 types of diffs:

-       mov      rax, rdx
+       mov      eax, edx

This is because codegen previously called inst_RV_RV without a type so it default to TYP_I_IMPL even when TYP_INT would have sufficed.

-       mov      rdx, rbx
        lock     
-       add      dword ptr [rcx], edx
+       add      dword ptr [rcx], ebx

This is because BuildNode was creating a definition for LOCKADD even if one wasn't actually needed.

LOCKADD nodes are generated rather early and there's no reason for that: * The CORINFO_INTRINSIC_InterlockedAdd32/64 intrinsics are not actually used. Even if they would be used we can still import them as XADD nodes and rely on lowering to generate LOCKADD when needed. * gtExtractSideEffList transforms XADD into LOCKADD but this can be done in lowering. LOCKADD is an XARCH specific optimization after all. Additionally: * Avoid the need for special handling in LSRA by making GT_LOCKADD a "no value" oper. * Split LOCKADD codegen from XADD/XCHG codegen, attempting to use the same code for all 3 just makes things more complex. * The address is always in a register so there's no real need to create an indir node on the fly, the relevant emitter functions can be called directly. The last point above is actually a CQ issue - we always generate `add [reg], imm`, more complex address modes are not used. Unfortunately this problem starts early, when the importer spills the address to a local variable. If that ever gets fixed then we'll could probably generate a contained LEA in lowering.

CarolEidt · 2018-06-04T21:56:36Z

This is a much more elegant solution than I was envisioning - I love it when the best solution actually simplifies things instead of adding more complexity.
I don't see any reason not to #14557 after this - do you?

CarolEidt

LGTM

mikedn · 2018-06-05T00:46:55Z

I don't see any reason not to #14557 after this - do you?

Up to you, this change does not prevent adding an indir if we find that's valuable.

That said, I went again through #14547 (where the issue started) and it looks like I've made a mistake. I dropped a getActualType from LOCKADD data node but that was supposed to be done only for XADD/XCHG. Strange that no test failed, I'll have to investigate.

mikedn · 2018-06-05T00:58:11Z

src/jit/codegenxarch.cpp

-    noway_assert(addrReg != targetReg);
+    GenTree* addr = node->gtGetOp1();
+    GenTree* data = node->gtGetOp2();
+    emitAttr size = emitTypeSize(data->TypeGet());


This must be emitActualTypeSize, otherwise the assert bellow will likely fail if the data operand is a small int indir (or any other node that may be small int).

We have a test for such a situation but it uses exchange rather than add so it does not catch this: https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/Regression/JitBlue/GitHub_10714/GitHub_10714.cs (funny, I added that but I don't remember it).

mikedn · 2018-06-05T00:59:35Z

src/jit/lower.cpp

+                node->ClearUnusedValue();
+                // Make sure the types are identical, since the node type is changed to VOID
+                // CodeGen relies on op2's type to determine the instruction size.
+                assert(node->gtGetOp2()->TypeGet() == node->TypeGet());


This needs to be genActualType(gtGetOp2()->TypeGet()) == node->TypeGet().

mikedn · 2018-06-05T19:38:46Z

Fix for actual type mistake available in #18303. The issue is mostly theoretical, in practice it doesn't seem possible to hit this.

Port #18267 fix value numbering when selecting a constant to release/2.2

Cleanup LOCKADD handling Commit migrated from dotnet/coreclr@e8661fe

mikedn force-pushed the lockadd3 branch from 7feb238 to 0a1a010 Compare June 4, 2018 15:44

mikedn force-pushed the lockadd3 branch from 0a1a010 to 9adb985 Compare June 4, 2018 18:38

mikedn changed the title ~~[WIP] Cleanup LOCKADD handling~~ Cleanup LOCKADD handling Jun 4, 2018

CarolEidt approved these changes Jun 4, 2018

View reviewed changes

CarolEidt merged commit e8661fe into dotnet:master Jun 4, 2018

mikedn commented Jun 5, 2018

View reviewed changes

sdmaclea mentioned this pull request Jun 7, 2018

[Arm64] JIT generate LSE Atomics #18130

Merged

AndyAyersMS added a commit that referenced this pull request Nov 28, 2018

Merge pull request #21228 from AndyAyersMS/Port18235ToRelease2.2

8429614

Port #18267 fix value numbering when selecting a constant to release/2.2

mikedn deleted the lockadd3 branch March 9, 2019 20:37

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#18267 from mikedn/lockadd3

3639ddc

Cleanup LOCKADD handling Commit migrated from dotnet/coreclr@e8661fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup LOCKADD handling #18267

Cleanup LOCKADD handling #18267

Uh oh!

mikedn commented Jun 3, 2018 •

edited

Loading

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

CarolEidt commented Jun 4, 2018

Uh oh!

CarolEidt left a comment

Uh oh!

mikedn commented Jun 5, 2018

Uh oh!

mikedn Jun 5, 2018

Uh oh!

mikedn Jun 5, 2018

Uh oh!

mikedn commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Cleanup LOCKADD handling #18267

Cleanup LOCKADD handling #18267

Uh oh!

Conversation

mikedn commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

mikedn commented Jun 4, 2018

Uh oh!

CarolEidt commented Jun 4, 2018

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

mikedn commented Jun 5, 2018

Uh oh!

mikedn Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

mikedn Jun 5, 2018

Choose a reason for hiding this comment

Uh oh!

mikedn commented Jun 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mikedn commented Jun 3, 2018 •

edited

Loading