Skip to content

ILCompiler: Fix values of IsSupported for HW intrinsics on 32-bit platforms #99753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

ILCompiler: Fix values of IsSupported for HW intrinsics on 32-bit platforms #99753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0db9581
ILCompiler: Fix values of IsSupported for HW intrinsics on 32-bit pla…
filipnavara Mar 14, 2024
3935c7b
Fix out discard
filipnavara Mar 14, 2024
ba518fb
Merge branch 'main' into ilc-hw-ins
jkotas Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/Compiler/HardwareIntrinsicHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public static partial class HardwareIntrinsicHelpers
/// </summary>
public static bool IsHardwareIntrinsic(MethodDesc method)
{
return !string.IsNullOrEmpty(InstructionSetSupport.GetHardwareIntrinsicId(method.Context.Target.Architecture, method.OwningType));
return !string.IsNullOrEmpty(InstructionSetSupport.GetHardwareIntrinsicId(method.Context.Target.Architecture, method.OwningType, out _));
}

public static void AddRuntimeRequiredIsaFlagsToBuilder(InstructionSetSupportBuilder builder, int flags)
Expand Down
28 changes: 11 additions & 17 deletions src/coreclr/tools/Common/Compiler/InstructionSetSupport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,38 +64,32 @@ public bool IsInstructionSetExplicitlyUnsupported(InstructionSet instructionSet)

public InstructionSetSupportFlags Flags => _flags;

public static string GetHardwareIntrinsicId(TargetArchitecture architecture, TypeDesc potentialTypeDesc)
public static string GetHardwareIntrinsicId(TargetArchitecture architecture, TypeDesc potentialTypeDesc, out bool unsupportedSubset)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this returning a string, rather than simply some enum entry like is done for RyuJIT?

A string is expensive to compare against and it doesn't even look like this is returning an Intrinsic ID, but rather the type name. A type name by itself may or may not be sufficient for describing an actual InstructionSet and whether or not its supported. This is why we have distinct InstructionSet enum entries for each class we can encounter in RyuJIT.

{
unsupportedSubset = false;

if (!potentialTypeDesc.IsIntrinsic || !(potentialTypeDesc is MetadataType potentialType))
return "";

if (architecture == TargetArchitecture.X64)
{
if (potentialType.Name == "X64")
potentialType = (MetadataType)potentialType.ContainingType;
if (potentialType.Name == "VL")
potentialType = (MetadataType)potentialType.ContainingType;
if (potentialType.Namespace != "System.Runtime.Intrinsics.X86")
return "";
}
else if (architecture == TargetArchitecture.X86)
if (architecture is TargetArchitecture.X64 or TargetArchitecture.X86)
{
if (potentialType.Name == "X64")
{
potentialType = (MetadataType)potentialType.ContainingType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this more complex change rather than just having this line change to return ""; for architecture == TargetArchitecture.X86?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you simply exclude it then we end up with IsSupported and the actual HW intrinsic methods compiled from the IL code, ie. recursive calls that cause stack overflow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it really seems this should be doing some resolution similar to what lookupId does in RyuJIT and deciding the IL replacement logic based on the enum value returned and not some vague estimation.

It looks like you don't need full resolution, rather you mostly just care about:

  • What ISA is the intrinsic part of?
  • Is it one of the special intrinsics (get_IsSupported, get_IsHardwareAccelerated, get_Count, etc)?
  • Is it recursive?

unsupportedSubset = architecture == TargetArchitecture.X86;
}
if (potentialType.Name == "VL")
potentialType = (MetadataType)potentialType.ContainingType;
if (potentialType.Namespace != "System.Runtime.Intrinsics.X86")
return "";
}
else if (architecture == TargetArchitecture.ARM64)
else if (architecture is TargetArchitecture.ARM64 or TargetArchitecture.ARM)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no intrinsic support for TargetArchitecture.ARM today (by any of our runtimes). Does this need to report the type or can it accordingly just report it as "unsupported" for the time being?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I folded it to follow the same logic as X64/X86. The ARM logic existed there before the change.

{
if (potentialType.Name == "Arm64")
{
potentialType = (MetadataType)potentialType.ContainingType;
if (potentialType.Namespace != "System.Runtime.Intrinsics.Arm")
return "";
}
else if (architecture == TargetArchitecture.ARM)
{
unsupportedSubset = architecture == TargetArchitecture.ARM;
}
if (potentialType.Namespace != "System.Runtime.Intrinsics.Arm")
return "";
}
Expand Down
17 changes: 14 additions & 3 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/HardwareIntrinsicILProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,25 @@ public HardwareIntrinsicILProvider(InstructionSetSupport isaSupport, FieldDesc i
public override MethodIL GetMethodIL(MethodDesc method)
{
TypeDesc owningType = method.OwningType;
string intrinsicId = InstructionSetSupport.GetHardwareIntrinsicId(_context.Target.Architecture, owningType);
string intrinsicId = InstructionSetSupport.GetHardwareIntrinsicId(_context.Target.Architecture, owningType, out bool unsupportedSubset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just trying to replace the method body for a couple of the special cases?

Can we not just have this logic match what RyuJIT does so we can handle the total set of special "constant" cases:

  • NI_IsSupported_True
  • NI_IsSupported_False
  • NI_IsSupported_Dynamic
  • NI_IsSupported_Type
  • NI_Throw_PlatformNotSupportedException
  • NI_Vector_GetCount

if (!string.IsNullOrEmpty(intrinsicId)
&& HardwareIntrinsicHelpers.IsIsSupportedMethod(method))
{
InstructionSet instructionSet = _instructionSetMap[intrinsicId];

bool isSupported = _isaSupport.IsInstructionSetSupported(instructionSet);
bool isOptimisticallySupported = _isaSupport.OptimisticFlags.HasInstructionSet(instructionSet);
bool isSupported;
bool isOptimisticallySupported;

if (unsupportedSubset)
{
isSupported = false;
isOptimisticallySupported = false;
}
else
{
isSupported = _isaSupport.IsInstructionSetSupported(instructionSet);
isOptimisticallySupported = _isaSupport.OptimisticFlags.HasInstructionSet(instructionSet);
}

// If this is an instruction set that is optimistically supported, but is not one of the
// intrinsics that are known to be always available, emit IL that checks the support level
Expand Down