Mask count in shift operations on native integers by jcouv · Pull Request #61341 · dotnet/roslyn

jcouv · 2022-05-16T06:35:00Z

tannergooding · 2022-05-17T00:31:36Z

Does this need a corresponding "breaking change" doc?

Notably x86/x64 performs "masked shifting" already and so nint << x was already effectively being handled as int << (x & 0x1F) on 32-bit and long << (x & 0x3F) on 64-bit. It looks like Arm64 does the same and so I would expect that next to no customers are actually impacted by this change.

However, some platform supported by another runtime might see differing behavior when compiled with C# 11 vs when compiled with a prior version since nint isn't emitting the appropriate masking required for determinism today.

jcouv · 2022-05-17T02:52:17Z

@tannergooding I was not planning to add a breaking change entry for this fix. But we can discuss it during code review.

AlekseyTs · 2022-05-25T14:11:20Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            // - When the type of `x` is `nint` or `nuint`, the shift count is given by
+            //   the low-order five bits of `count` on a 32 bits platform, or
+            //   the lower-order six bits of `count` on a 64 bits platform.
+            //   The shift count is computed from `count & (sizeof(nint) * 8 - 1)` (or `nuint`),


(or nuint)

I am not sure what this part is supposed to convey. #Closed

AlekseyTs · 2022-05-25T14:12:56Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            // - When the type of `x` is `nint` or `nuint`, the shift count is given by
+            //   the low-order five bits of `count` on a 32 bits platform, or
+            //   the lower-order six bits of `count` on a 64 bits platform.
+            //   The shift count is computed from `count & (sizeof(nint) * 8 - 1)` (or `nuint`),


computed from

"computed as" rather than "computed from"? #Closed

AlekseyTs · 2022-05-25T14:17:09Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            TypeSymbol rightType = loweredRight.Type;
+            Debug.Assert(rightType.SpecialType == SpecialType.System_Int32);
+
+            if (rightConstantValue != null && rightConstantValue.IsIntegral && rightConstantValue.Int32Value <= 0x1F)


IsIntegral

IsIntegral actually checks for all integral types. Perhaps we should check Discriminator == ConstantValueTypeDiscriminator.Int32 instead. #Closed

AlekseyTs · 2022-05-25T14:28:50Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            }
+
+            return oldNode == null
+                ? _factory.Binary(operatorKind, type, loweredLeft, loweredRight)


_factory.Binary(operatorKind, type, loweredLeft, loweredRight)

Consider using syntax from the context as RewriteBuiltInShiftOperation does. #Closed

AlekseyTs · 2022-05-25T14:33:58Z

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs

        }
+
+        [Fact, WorkItem(43347, "https://github.com/dotnet/roslyn/issues/43347")]
+        public void MaskShiftCount()


MaskShiftCount

Is this a dupe of a test added to NumericIntPtrTests.cs? #Closed

We have tests for old native integers (ie. using a runtime without feature flag) and new native integers (on new runtime).
This file is for the old native integers (fix applies to them too).

AlekseyTs · 2022-05-25T14:36:19Z

Done with review pass (commit 3)

cston · 2022-05-26T17:22:50Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            // For the predefined operators, the number of bits to shift in `x << count`, `x >> count` or `x >>> count` is computed as follows:
+            // [...]
+            // - When the type of `x` is `nint` or `nuint`, the shift count is given by
+            //   the low-order five bits of `count` on a 32 bits platform, or


32 bits platform

Consider using "... bit platform" (singular "bit") throughout this comment to match the spec. #Closed

cston · 2022-05-26T17:30:31Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            TypeSymbol rightType = loweredRight.Type;
+            Debug.Assert(rightType.SpecialType == SpecialType.System_Int32);
+
+            if (rightConstantValue != null && rightConstantValue.IsIntegral && rightConstantValue.Int32Value <= 0x1F)


rightConstantValue.Int32Value <= 0x1F

Are we checking rightConstantValue >= 0? #Closed

Thanks. Added

cston · 2022-05-26T17:37:04Z

src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs

+            validate("nuint", "NUintMaxValue", ">>> 63", "0x0000_0001", "0x0000_0000_0000_0001", nuint_shr_un(63));
+            validate("nuint", "NUintMaxValue", ">>> 64", "0xFFFF_FFFF", "0xFFFF_FFFF_FFFF_FFFF", nuint_shr_un(64));
+
+            // lifted value (sampler)


(sampler)

What does "sampler" mean here and below? #Closed

It was meant as "not as exhaustive" (just picking a few interesting permutations). I'll remove

cston · 2022-05-26T19:08:45Z

src/Compilers/CSharp/Test/Semantic/Semantics/NativeIntegerTests.cs

+                //         return x op count;
+                Diagnostic(ErrorCode.ERR_BadBinaryOps, $"x {op} count").WithArguments(op, type, "int").WithLocation(5, 16)
+                );
+        }


}

Are we masking the shift amount in the following?

class Program { static void Main() { System.Console.WriteLine(ShiftRight(255)); } static nint ShiftRight(nint x) => x >> (-62); } ``` #Resolved

AlekseyTs · 2022-05-27T20:20:33Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+
+            if (rightConstantValue != null
+                && rightConstantValue.Discriminator == ConstantValueTypeDiscriminator.Int32
+                && rightConstantValue.Int32Value is >= 0 and <= 0x1F)


rightConstantValue.Int32Value is >= 0 and <= 0x1F

Perhaps we will be able to handle some negative numbers as well with the following logic?

int originalValue = rightConstantValue.Int32Value; int maskedValue = originalValue & 0x1F; bool useMaskedValue = (originalValue & ~((1 << 31) | maskedValue )) == 0;

#Resolved

AlekseyTs · 2022-05-27T20:29:54Z

Done with review pass (commit 5)

AlekseyTs · 2022-05-27T22:49:30Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+                && rightConstantValue.Discriminator == ConstantValueTypeDiscriminator.Int32
+                && (rightConstantValue.Int32Value & 0b100_000) == 0)
+            {
+                // For cases where count doesn't have the sixth bit set, we statically know the masked count.


// For cases where count doesn't have the sixth bit set, we statically know the masked count.

It is not clear to me why this is the case. What about shifting by 64 on 128bit platform? #Closed

nint becoming 128-bits would require revisions to the PE metadata format, the general IL support, and a number of other things. At this point, such a feature would break much existing .NET IL code (particularly given the prevalence of code like if (IntPtr.Size == 8) { } else { /* assume 32-bit */ } at some of the lowest layers)

Supporting a 128-bit platform, if it ever happens, is likely many years out. It wouldn't be unreasonable for the current compiler to just pretend it can't happen given the likelihood that it would require a break to the existing metadata format.

It also wouldn't be unreasonable to just guard against "too big shifts" or even emit a warning wave in such scenarios to help tell the user "this might not do what you think it does" (many users think that overshifting results in 0, like it does for byte, sbyte, short, and ushort when the mask is less than 31).

I can appreciate the trouble that runtime would have to go through, but I do not see a good reason for the compiler to do so as well (I guess apart from supporting new metadata format, if that would be even needed). We can easily have correct implementation that doesn't depend on the assumption that 64bit is the biggest size we will ever encounter. I do not see this assumption "helping" compiler today or in the long run.

Reverted to simpler logic (only optimize if count is >= 0 and <= 0x1F). This all isn't worth the effort to optimize for odd cases of negative counts.

This reverts commit 7dd0d62.

AlekseyTs

LGTM (commit 9)

jcouv added Area-Compilers Feature - Native Int labels May 16, 2022

jcouv self-assigned this May 16, 2022

jcouv force-pushed the mask-shift branch 2 times, most recently from 0aa2160 to 09ae707 Compare May 16, 2022 09:06

jcouv force-pushed the mask-shift branch 2 times, most recently from 2633235 to 0873c0a Compare May 17, 2022 02:51

jcouv force-pushed the mask-shift branch from 0873c0a to fde4cbc Compare May 18, 2022 05:10

Mask count in shift operations on native integers

b34aae0

jcouv force-pushed the mask-shift branch from fde4cbc to 2343011 Compare May 20, 2022 16:10

runfoapp bot mentioned this pull request May 20, 2022

roslyn.visualstudio.newintegrationtests.csharp.csharperrorlistnetcore.errorlist #61367

Open

Adjust tests

d87bcde

jcouv force-pushed the mask-shift branch from 2343011 to d87bcde Compare May 23, 2022 23:52

tweak

6f862aa

jcouv force-pushed the mask-shift branch from 86c10ab to 6f862aa Compare May 24, 2022 04:41

jcouv marked this pull request as ready for review May 25, 2022 05:50

jcouv requested a review from a team as a code owner May 25, 2022 05:50

AlekseyTs reviewed May 25, 2022

View reviewed changes

cston reviewed May 26, 2022

View reviewed changes

Address feedback

521ecb9

Address feedback from Chuck

876a496

AlekseyTs reviewed May 27, 2022

View reviewed changes

Statically mask where possible

7dd0d62

jcouv requested a review from AlekseyTs May 27, 2022 22:33

AlekseyTs reviewed May 27, 2022

View reviewed changes

Julien Couvreur added 2 commits May 27, 2022 16:40

Revert "Statically mask where possible"

2428c90

This reverts commit 7dd0d62.

bit

edca598

jcouv requested a review from AlekseyTs June 1, 2022 07:47

Merge remote-tracking branch 'dotnet/main' into mask-shift

639e9b4

jcouv requested a review from cston June 1, 2022 16:39

cston approved these changes Jun 1, 2022

View reviewed changes

AlekseyTs approved these changes Jun 1, 2022

View reviewed changes

jcouv merged commit 9c99ebe into dotnet:main Jun 1, 2022

ghost added this to the Next milestone Jun 1, 2022

jcouv deleted the mask-shift branch June 1, 2022 18:39

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

Conversation

jcouv commented May 16, 2022

Uh oh!

tannergooding commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented May 17, 2022

Uh oh!

AlekseyTs May 25, 2022 • 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!

AlekseyTs May 25, 2022 • 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!

AlekseyTs May 25, 2022 • 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!

AlekseyTs May 25, 2022 • 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!

AlekseyTs May 25, 2022 • 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!

jcouv May 27, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 25, 2022

Uh oh!

cston May 26, 2022 • 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!

cston May 26, 2022 • 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!

jcouv May 27, 2022

Choose a reason for hiding this comment

Uh oh!

cston May 26, 2022 • 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!

jcouv May 27, 2022

Choose a reason for hiding this comment

Uh oh!

cston May 26, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs May 27, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented May 27, 2022

Uh oh!

AlekseyTs May 27, 2022 • 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!

tannergooding May 27, 2022 • 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!

AlekseyTs May 27, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv May 27, 2022

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

tannergooding commented May 17, 2022 •

edited

Loading

AlekseyTs May 25, 2022 •

edited

Loading

AlekseyTs May 25, 2022 •

edited

Loading

AlekseyTs May 25, 2022 •

edited

Loading

AlekseyTs May 25, 2022 •

edited

Loading

AlekseyTs May 25, 2022 •

edited

Loading

cston May 26, 2022 •

edited

Loading

cston May 26, 2022 •

edited

Loading

cston May 26, 2022 •

edited

Loading

cston May 26, 2022 •

edited by jcouv

Loading

AlekseyTs May 27, 2022 •

edited by jcouv

Loading

AlekseyTs May 27, 2022 •

edited

Loading

tannergooding May 27, 2022 •

edited

Loading