Add bit shift intrinsic functions

[jrdodds]

Fixes #8551

Context

The intrinsic functions include functions for bit And, Or, Xor, and Not operators but not for the bit shift operators.

Changes Made

Added functions LeftShift, RightShift, and RightShiftUnsigned. The functions expect values of type int (32 bit) which matches the existing bit operator functions. long (64 bit) is not supported by any of the bit operator intrinsic functions.

Testing

Tested on Windows 11 and macOS 12 Monterey.

Added to Expander_Tests.cs and ran unit tests.

Created and ran BitShift.proj project file with the following content:

<?xml version="1.0" encoding="UTF-8"?>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

  <Target Name="Test">
    <PropertyGroup>
      <LeftShiftTest>$([MSBuild]::LeftShift(1, 2))</LeftShiftTest>
      <RightShiftTest>$([MSBuild]::RightShift(-8, 2))</RightShiftTest>
      <RightShiftUnsignedTest>$([MSBuild]::RightShiftUnsigned(-8, 2))</RightShiftUnsignedTest>
    </PropertyGroup>

    <Message Text="LeftShiftTest: $(LeftShiftTest)" />
    <Message Text="RightShiftTest: $(RightShiftTest)" />
    <Message Text="RightShiftUnsignedTest: $(RightShiftUnsignedTest)" />
  </Target>

</Project>

The project produced the following output (which matched the expected results):

Test:
  LeftShiftTest: 4
  RightShiftTest: -2
  RightShiftUnsignedTest: 1073741822

Notes

[Forgind]

Thanks!

[Forgind]

I wasn't aware this existed. Always nice to learn something new!

[rainersigwald]

Looks good! Can you please add a "fast path" in this block to avoid having to use reflection when this is called?

msbuild/src/Build/Evaluation/Expander.cs

Lines 3826 to 3835 in 60ea2f7

	else if (_receiverType == typeof(IntrinsicFunctions))
	{
	if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.EnsureTrailingSlash), StringComparison.OrdinalIgnoreCase))
	{
	if (TryGetArg(args, out string arg0))
	{
	returnVal = IntrinsicFunctions.EnsureTrailingSlash(arg0);
	return true;
	}
	}

[ladipro]

@rainersigwald, the existing bitwise operations are not on the fast path either. Presumably because they're not expected to be used very often and, while making the first call faster, every such special case if incurs a small steady state perf hit.

[rainersigwald]

In the past we've added any method that is known to be called in the wild to the fast path, but haven't required it, and I suspect the bitwise operations were missed, rather than intentionally omitted.

[jrdodds]

I added a "fast path" for each of the bit operator functions:

• BitwiseOr
• BitwiseAnd
• BitwiseXor
• BitwiseNot
• LeftShift
• RightShift
• RightShiftUnsigned

In Expander_Tests I separated the bit operator functions into their own test method from the math functions, and I changed from Xunit Assert's to Shouldly. (I was motivated to make the unit test change because I had a typo and the Xunit Assert message didn't include the name of the failing function.)

[ladipro]

@rainersigwald, the existing bitwise operations are not on the fast path either. Presumably because they're not expected to be used very often and, while making the first call faster, every such special case if incurs a small steady state perf hit.

Make sense. Also, source generators for the win! It should be a fun project to automatically create efficient lookup tables at compile time.

[rainersigwald]

Also, source generators for the win! It should be a fun project to automatically create efficient lookup tables at compile time.

I have been fantasizing about doing this for several years now!

[jrdodds]

I'm looking at the 'Fast Path' TryExecuteWellKnownFunction code for other functions and I have some questions.

IntrinsicFunctions.cs includes Add functions for double and int:

msbuild/src/Build/Evaluation/IntrinsicFunctions.cs

Lines 40 to 55 in b84faa7

	/// <summary>
	/// Add two doubles
	/// </summary>
	internal static double Add(double a, double b)
	{
	return a + b;
	}
	/// <summary>
	/// Add two longs
	/// </summary>
	internal static long Add(long a, long b)
	{
	return a + b;
	}

The TryExecuteWellKnownFunction method in Expander.cs tests for but doesn't use the Add functions:

msbuild/src/Build/Evaluation/Expander.cs

Lines 3893 to 3900 in b84faa7

	else if (string.Equals(_methodMethodName, nameof(IntrinsicFunctions.Add), StringComparison.OrdinalIgnoreCase))
	{
	if (TryGetArgs(args, out double arg0, out double arg1))
	{
	returnVal = arg0 + arg1;
	return true;
	}
	}

Subtract, Multiply, and Divide have the same pattern. The operator is used instead of invoking the function.

There are Modulo functions for double and int and they are not in the TryExecuteWellKnownFunction method.

Shouldn't the TryExecuteWellKnownFunction method always be invoking the appropriate member of IntrinsicFunctions (and never directly using the operator)?

It seems that currently Add, Subtract, Multiply, and Divide are always performed with doubles. How are the int and double overloads intended to resolve? Does the 'slow path' resolve the type overload?

[Forgind]

Shouldn't the TryExecuteWellKnownFunction method always be invoking the appropriate member of IntrinsicFunctions (and never directly using the operator)?

I don't think this really matters. From a style perspective, it is better to have them all look the same, that is, all call the appropriate member of IntrinsicFunctions as you suggest. From the user's perspective, they're equivalent, and if the call isn't inlined, calling '+' directly may even be (negligibly) faster.

It seems that currently Add, Subtract, Multiply, and Divide are always performed with doubles. How are the int and double overloads intended to resolve? Does the 'slow path' resolve the type overload?

There aren't really "ints," "doubles," etc. in MSBuild; everything is just strings. Even things like TryConvertToInt are kinda misleading, as far as I know, because 'value' won't ever be a double or an int; it'll always be a string, and we'll always parse it as such. With that in mind, if you pass Add two ints, they just turn into doubles, get added, and get converted back to a string that looks like an integer, so people generally don't care too much about whether the arithmetic was actually executed with ints or doubles. Since all ints are doubles, though, we do get them in the fast path.

[jrdodds]

@Forgind It sounds like the int overloads of Add, Subtract, Multiply, Divide, and Modulo should be removed because they are never used.

[Forgind]

I wouldn't be opposed to that. The only ways I can think of that someone could actually call them is if they use reflection or MSBuildLocator to specifically load our IntrinsicFunctions class then call the int overload...neither feels at all worthwhile to me, since C# programmers don't need a special function to call something built-in. Can you think of any legitimate reason someone might want to use one of those overloads @rainersigwald?

[jrdodds]

It might be considered clean up for supporting the source generation @rainersigwald and @ladipro mentioned. 😊

[rainersigwald]

@jrdodds I'm not sure of the impact of some of those removals, and it's late enough in the 17.6 release cycle that I'm nervous about removing them. Can you separate them into a different PR that we can think about for a bit longer? Adding the shift operators and the fast paths is in good shape, though.

[jrdodds]

@rainersigwald This PR has been updated.