[Arm64] Pr jit acquire release simple cases by sdmaclea · Pull Request #12087 · dotnet/coreclr

sdmaclea · 2017-06-05T17:56:04Z

No description provided.

sdmaclea · 2017-06-05T17:56:37Z

@dotnet/arm64-contrib @dotnet/jit-contrib

sdmaclea · 2017-06-05T18:03:44Z

src/jit/emitarm64.cpp

-    if (code & 0x00800000) // Is this a sign-extending opcode? (i.e. ldrsw, ldrsh, ldrsb)
+    bool exclusive = ((code & 0x35000000) == 0);
+
+    if ((code & 0x00800000) && !exclusive) // Is this a sign-extending opcode? (i.e. ldrsw, ldrsh, ldrsb)


The original code was not fully decoding whether this was a sign extending load.

Acquire/Release are in the exclusive group which does not support sign extending forms. So this is needed to correctly generate some of the store release forms..

The decode here is still incomplete, but it is sufficient for our current set of generated instructions. I might expect similar issues when we try to support ARM 8.1 atomics...

briansull · 2017-06-05T18:08:36Z

src/jit/codegenarm64.cpp

-            // issue a full memory barrier a before volatile StInd
-            instGen_MemoryBarrier();
+            bool useStoreRelease = (dataReg >= REG_INT_FIRST) && (dataReg <= REG_ZR) && !addr->isContained() &&
+                                   ((attr == EA_1BYTE) || !(tree->gtFlags & GTF_IND_UNALIGNED));


Isn't there already a method that validates that we have a General purpose register that we can use for this ?
(dataReg >= REG_INT_FIRST) && (dataReg <= REG_ZR)

We don't want to bake in the fact that REG_ZR has the largest enum value for a general purpose register in this code.

It also should not be possible to have the case of an "unaligned" 1 byte load/store.
A check could be added when we decide to set that flag.

Looks like genIsValidIntReg(regNumber reg) is a direct replacement. I'll use that unless you strongly prefer not float.

It also should not be possible to have the case of an "unaligned" 1 byte load/store.
A check could be added when we decide to set that flag.

My original code added an assert unaligned not set on byte size code which triggered on some of the directed volatile prefix tests.

Since Unaligned is a IL prefix, I think we do not remove the prefix in import. I expect some of the directed volatile prefix tests set the unaligned flag on byte sized load/store. We can add code to clear the flag during import for byte sized ops. I do not think the prefix on a byte load/store is explicitly illegal IL, so ignoring it would be the best course of action IMO.

Either way I'll remove the special case here.

briansull · 2017-06-05T18:21:28Z

src/jit/codegenarmarch.cpp

-        instGen_MemoryBarrier(INS_BARRIER_LD);
+        GenTree* addr           = tree->Addr();
+        bool     useLoadAcquire = (targetReg >= REG_INT_FIRST) && (targetReg <= REG_ZR) && !addr->isContained() &&
+                              varTypeIsUnsigned(targetType) &&


I think that the register check can be changed to a check that targetType is not a floating point type.

sdmaclea · 2017-06-05T20:36:45Z

@briansull This should address your comments.

briansull · 2017-06-05T21:33:03Z

src/jit/importer.cpp

                        }
                    }

-                    if (prefixFlags & PREFIX_UNALIGNED)


I don't understand the code that follow this, so I would just leave this line alone.

But you also might want to correct the broken indention here...

This code just sets the GTF_IND_UNALIGNED if it is not using a helper.

Not sure what indentation is broken. clang-format was happy with it. Perhaps you missed the discontinuityin the patch line numbers?

Yeah, The diff does have a big discontinuity in the line numbers. I have never seen that before.
Makes it hard to code review.

briansull · 2017-06-05T21:34:20Z

src/jit/importer.cpp

                        op1->gtFlags |= GTF_IND_VOLATILE;
                    }
-                    if (prefixFlags & PREFIX_UNALIGNED)
+                    if ((prefixFlags & PREFIX_UNALIGNED) && !varTypeIsByte(lclTyp))


This change is fine, as we just want to set things up so that we don't set GTF_IND_UNALIGNED when we have a byte sized indirection

briansull · 2017-06-05T21:35:53Z

src/jit/importer.cpp

@@ -13594,7 +13594,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                        op1->gtFlags |= GTF_ORDER_SIDEEFF; // Prevent this from being reordered
                        op1->gtFlags |= GTF_IND_VOLATILE;


It seems unnecessary to me to set these two flags when we see a UNALIGNED prefix, but that isn't part of your change.

Those are set when we see a VOLATILE prefix.

sdmaclea · 2017-06-06T13:07:23Z

@dotnet-bot test Ubuntu arm Cross Release Build

sdmaclea · 2017-06-08T00:08:16Z

Please merge

jashook · 2017-08-15T16:06:30Z

@sdmaclea the assert you added here is firing. It seems to me like it may be too restrictive. Is the assert: assert((attr != EA_1BYTE) || !(tree->gtFlags & GTF_IND_UNALIGNED)); meant for the volatile case or in general?

sdmaclea · 2017-08-15T16:42:45Z

@jashook it was meant for the general case. The GTF_IND_UNALIGNED flag is not supposed to be set on EA_1BYTE objects because they can by definition never be unaligned.

If the assertion is removed the volatile code will have to be changed. the correct fix would be to not set the Unaligned flag for these objects.

sdmaclea added 3 commits June 5, 2017 17:39

[Arm64] Use Load Acquire when practical for genCodeForIndir

0d8bba3

[Arm64] Use Store Release when practical for genCodeForStoreInd

e311d87

[Arm64] Handle exclusive decode group in insEncodeDatasizeLS()

69b057f

dnfclas added the cla-already-signed label Jun 5, 2017

sdmaclea commented Jun 5, 2017

View reviewed changes

briansull suggested changes Jun 5, 2017

View reviewed changes

briansull reviewed Jun 5, 2017

View reviewed changes

sdmaclea added 2 commits June 5, 2017 20:31

[Arm64] Respond to review comments

b0455ff

Do not set GTF_IND_UNALIGNED on byte sized types

2011e0b

briansull approved these changes Jun 5, 2017

View reviewed changes

CarolEidt merged commit 1a6c1d1 into dotnet:master Jun 8, 2017

sdmaclea deleted the PR-JIT-AcquireRelease-SimpleCases branch August 1, 2017 17:58

karelz modified the milestone: 2.1.0 Aug 28, 2017

sdmaclea mentioned this pull request Apr 14, 2018

[Arm64] Fix WorkStealingQueue memory ordering #17508

Closed

sdmaclea mentioned this pull request Jan 31, 2020

[Arm64] Harder Acquire Release cases dotnet/runtime#10149

Closed

		@@ -13594,7 +13594,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
		op1->gtFlags \|= GTF_ORDER_SIDEEFF; // Prevent this from being reordered
		op1->gtFlags \|= GTF_IND_VOLATILE;

Conversation

sdmaclea commented Jun 5, 2017

Uh oh!

sdmaclea commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea commented Jun 6, 2017

Uh oh!

sdmaclea commented Jun 8, 2017

Uh oh!

jashook commented Aug 15, 2017

Uh oh!

sdmaclea commented Aug 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sdmaclea Jun 5, 2017 •

edited

Loading