Fix HW intrinsic containment bugs#23558
Conversation
For the Fma case (dotnet#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
|
Just as an FYI, @jkotas logged another bug for |
| regSet.tmpRlsTemp(tmpDsc); | ||
| } | ||
| else if (op3->OperIsHWIntrinsic()) | ||
| else if (op3->isIndir() || op3->OperIsHWIntrinsic()) |
There was a problem hiding this comment.
Are similar fixes needed for all the other genHWIntrinsic_ code? Such as:
coreclr/src/jit/hwintrinsiccodegenxarch.cpp
Line 435 in 4ff4ef1
There was a problem hiding this comment.
I suspect so. However, I will file a separate issue that includes extending the testing to include more containment cases.
There was a problem hiding this comment.
If it is just this same thing on all of them, I can address that and you can feel free to assign the issue to me.
There was a problem hiding this comment.
Right - it would be just the same, but I don't want to introduce the code without a test to verify it (plus I thought it would be prudent to do it separately, to keep this focused on fixing the existing failures).
| { | ||
| return -1; | ||
| float result = fmaTest(); | ||
| if (Math.Abs(result - 5.0F) > System.Single.Epsilon) |
There was a problem hiding this comment.
I would say we can compare this against Math.FusedMultiplyAdd now; but that will get accelerated if Fma.IsSupported returns true
There was a problem hiding this comment.
I'd just as soon be explicit here
There was a problem hiding this comment.
If you are wanting to be explicit, then this result should be deterministic and you shouldn't need anything more than result != 5.0f.
If this isn't deterministic, then we have other problems 😄
|
@tannergooding @dotnet/jit-contrib PTAL |
|
@dotnet-bot test Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness |
| <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/Regression/GitHub_21899/**"> | ||
| <Issue>23534</Issue> | ||
| </ExcludeList> | ||
| <ExcludeList Include="$(XunitTestBinBase)/JIT/Methodical/flowgraph/dev10_bug679955/volatileLocal1/*"> |
There was a problem hiding this comment.
You deleted the exclusion for the test that you have not fixed. #23545 is still open.
There was a problem hiding this comment.
Yes, bad/mindless merge. I've pushed a PR to re-exclude it.
* Fix HW intrinsic containment bugs For the Fma case (dotnet#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
* Fix HW intrinsic containment bugs For the Fma case (dotnet/coreclr#23430), fix the handling of contained 3-operand HW intrinsic nodes. For the Bmi case (dotnet/coreclr#23534), fix a bad assert placement, and re-enable the Bmi tests. Fix dotnet/coreclr#23530 Fix dotnet/coreclr#23534 * Add guard for Fma test Commit migrated from dotnet/coreclr@1df87c7
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