JIT: Don't use addressing modes for volatile loads for gc types

[EgorBo]

No description provided.

[ghost]

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

Issue Details

---

null

Author:	EgorBo
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc

[azure-pipelines]

Azure Pipelines successfully started running 4 pipeline(s).

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc

[EgorBo]

Diff is a size regression as expected: https://dev.azure.com/dnceng-public/public/_build/results?buildId=355867&view=ms.vss-build-web.run-extensions-tab

E.g.

-            mov     w2, #0xD1FFAB1E
-            dmb     ish
-            stp     w2, w1, [x0, #0x34]
+            add     x2, x0, #52
+            mov     w3, #0xD1FFAB1E
+            stlr    w3, [x2]
+            str     w1, [x0, #0x38]
             ldp     fp, lr, [sp], #0x10
             ret     lr
-;Total bytes of code 28
+;Total bytes of code 32

We no longer can merge two stores with stp because on of the is volatile (the other is not) -- it actually smells like a potential bug in Main - should we even merge stores/load together for volatile?

Another kind of size regressions where no longer can use addressing modes becuase ldlr/stlr don't support them. This can be mitigated with #64457

But overall it should be a perf win because memory barriers are heavy.

PTAL @AndyAyersMS

[AndyAyersMS]

Did you get a chance to try it out on this case where it should help perf? #87194 (comment)

If not, I can do it.

[EgorBo]

Did you get a chance to try it out on this case where it should help perf? #87194 (comment)

If not, I can do it.

Method	Job	Toolchain	Size	Mean	Error	StdDev	Median	Min	Max	Ratio	Gen0	Gen1	Gen2	Allocated	Alloc Ratio
ConcurrentBag	Job-QLVLMF	/Core_Root/corerun	512	7.115 us	0.1261 us	0.1179 us	7.073 us	6.969 us	7.312 us	0.89	1.9496	0.9607	0.0283	16.16 KB	1.00
ConcurrentBag	Job-CQJZMM	/Core_Root_base/corerun	512	7.978 us	0.0392 us	0.0327 us	7.983 us	7.891 us	8.024 us	1.00	1.9770	0.9885	0.0319	16.16 KB	1.00

so 11% improvement on Apple M2 Max, but it's a completely different CPU from the one in the regression

[AndyAyersMS]

We don't have ampere data yet, but you can see this fixed the regression on the surface

[DrewScoggins]

Looks like this was not fixed, at least on Ubuntu Ampere hw.

[EgorBo]

Looks like this was not fixed, at least on Ubuntu Ampere hw.

Lots of benchmarks regressed because of this PR, but that is expected - it was a BDN issue: BDN uses volatile to store results of benchmarks in a loop so we optimized the "overhead" part of benchmarks, there was an thread discussing it somewhere..