Fix HW intrinsic containment bugs (#23558)

* Fix HW intrinsic containment bugs

For the Fma case (#23430), fix the handling of contained 3-operand HW intrinsic nodes.
For the Bmi case (#23534), fix a bad assert placement, and re-enable the Bmi tests.

Fix #23530
Fix #23534

* Add guard for Fma test

diff --git a/src/jit/hwintrinsiccodegenxarch.cpp b/src/jit/hwintrinsiccodegenxarch.cpp
index 4d63bef..37d551f 100644 (file)
--- a/src/jit/hwintrinsiccodegenxarch.cpp
+++ b/src/jit/hwintrinsiccodegenxarch.cpp
@@ -1045,39 +1045,47 @@ void CodeGen::genHWIntrinsic_R_R_R_RM(
 
             regSet.tmpRlsTemp(tmpDsc);
         }
-        else if (op3->OperIsHWIntrinsic())
+        else if (op3->isIndir() || op3->OperIsHWIntrinsic())
         {
-            emit->emitIns_SIMD_R_R_R_AR(ins, attr, targetReg, op1Reg, op2Reg, op3->gtGetOp1()->gtRegNum);
-            return;
-        }
-        else if (op3->isIndir())
-        {
-            GenTreeIndir* memIndir = op3->AsIndir();
-            GenTree*      memBase  = memIndir->gtOp1;
+            GenTree*      addr;
+            GenTreeIndir* memIndir = nullptr;
+            if (op3->isIndir())
+            {
+                memIndir = op3->AsIndir();
+                addr     = memIndir->Addr();
+            }
+            else
+            {
+                assert(op3->AsHWIntrinsic()->OperIsMemoryLoad());
+                assert(HWIntrinsicInfo::lookupNumArgs(op3->AsHWIntrinsic()) == 1);
+                addr = op3->gtGetOp1();
+            }
 
-            switch (memBase->OperGet())
+            switch (addr->OperGet())
             {
                 case GT_LCL_VAR_ADDR:
                 {
-                    varNum = memBase->AsLclVarCommon()->GetLclNum();
+                    varNum = addr->AsLclVarCommon()->GetLclNum();
                     offset = 0;
-
-                    // Ensure that all the GenTreeIndir values are set to their defaults.
-                    assert(!memIndir->HasIndex());
-                    assert(memIndir->Scale() == 1);
-                    assert(memIndir->Offset() == 0);
-
                     break;
                 }
 
                 case GT_CLS_VAR_ADDR:
                 {
-                    emit->emitIns_SIMD_R_R_R_C(ins, attr, targetReg, op1Reg, op2Reg, memBase->gtClsVar.gtClsVarHnd, 0);
+                    emit->emitIns_SIMD_R_R_R_C(ins, attr, targetReg, op1Reg, op2Reg, addr->gtClsVar.gtClsVarHnd, 0);
                     return;
                 }
 
                 default:
                 {
+                    if (memIndir == nullptr)
+                    {
+                        // This is the HW intrinsic load case.
+                        // Until we improve the handling of addressing modes in the emitter, we'll create a
+                        // temporary GT_IND to generate code with.
+                        GenTreeIndir load = indirForm(op3->TypeGet(), addr);
+                        memIndir          = &load;
+                    }
                     emit->emitIns_SIMD_R_R_R_A(ins, attr, targetReg, op1Reg, op2Reg, memIndir);
                     return;
                 }
@@ -2132,8 +2140,6 @@ void CodeGen::genBMI1OrBMI2Intrinsic(GenTreeHWIntrinsic* node)
         case NI_BMI2_MultiplyNoFlags:
         case NI_BMI2_X64_MultiplyNoFlags:
         {
-            // These do not support containment
-            assert(!op2->isContained());
             int numArgs = HWIntrinsicInfo::lookupNumArgs(node);
             assert(numArgs == 2 || numArgs == 3);
 
@@ -2168,6 +2174,8 @@ void CodeGen::genBMI1OrBMI2Intrinsic(GenTreeHWIntrinsic* node)
                 assert(lowReg != targetReg);
             }
 
+            // These do not support containment
+            assert(!op2->isContained());
             emitAttr attr = emitTypeSize(targetType);
             // mov the first operand into implicit source operand EDX/RDX
             if (op1Reg != REG_EDX)

diff --git a/tests/issues.targets b/tests/issues.targets
index d0bab00..0e51874 100644 (file)
--- a/tests/issues.targets
+++ b/tests/issues.targets
@@ -74,18 +74,6 @@
         <ExcludeList Include="$(XunitTestBinBase)/tracing/keyword/TwoKeywords/TwoKeywords/*">
             <Issue>23224, often fails with timeout in release</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Bmi2/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Bmi2.X64/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Regression/GitHub_21899/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/Methodical/flowgraph/dev10_bug679955/volatileLocal1/*">
-            <Issue>23545</Issue>
-        </ExcludeList>
     </ItemGroup>
 
     <!-- All Unix targets -->
@@ -558,15 +546,6 @@
         <ExcludeList Include="$(XunitTestBinBase)/JIT/Regression/JitBlue/GitHub_19601/Github_19601/*">
             <Issue>Needs Triage</Issue>
         </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Bmi2/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Bmi2.X64/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
-        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Regression/GitHub_21899/**">
-            <Issue>23534</Issue>
-        </ExcludeList>
     </ItemGroup>
 
     <!-- Unix arm64 specific -->

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.cs b/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.cs
new file mode 100644 (file)
index 0000000..0131eeb
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.cs
@@ -0,0 +1,50 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Numerics;
+using System.Runtime.CompilerServices;
+using System.Collections.Generic;
+using System.Diagnostics;
+using System.Runtime.Intrinsics;
+using System.Runtime.Intrinsics.X86;
+
+namespace GitHub_23530
+{
+    class Program
+    {
+        struct vec
+        {
+            public float f1;
+            public float f2;
+            public float f3;
+            public float f4;
+        }
+
+        static unsafe float fmaTest()
+        {
+            vec a;
+            var b = Vector128.Create(1f);
+            var c = Vector128.Create(2f);
+            var d = Vector128.Create(3f);
+
+            c = Fma.MultiplyAdd(Sse.LoadVector128((float*)&a), b, c);
+
+            return Sse.Add(c, d).ToScalar();
+        }
+
+        static int Main(string[] args)
+        {
+            if (Fma.IsSupported)
+            {
+                float result = fmaTest();
+                if (Math.Abs(result - 5.0F) > System.Single.Epsilon)
+                {
+                    return -1;
+                }
+            }
+            return 100;
+        }
+    }
+}

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.csproj
new file mode 100644 (file)
index 0000000..fb6ae36
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_23530/GitHub_23530.csproj
@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Release</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>