Arm64: Support additional condition checks in select nodes #78223
Conversation
Generating a FEQ or FNEU will generate an incorrect compare. With the optimizer code fixed, it's not possible to generate a test to trigger the code in codegen. However, the code as it is was wrong. Given it's untested, it might be better to replace it with some asserts? I've added some general compare consume tests to ensure everything is generated ok.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsGenerating a FEQ or FNEU can generate an incorrect compare. Firstly, the optimizer needs changing to ensure the NAN flag is correct. Once that is fixed, it's not possible to generate a test to trigger the code in codegen (as there are no CIL beq.un or bne instructions). However, the codegen code was wrong. Alternatively, given it's untested, it might be better to replace the codegen changes with some asserts? I've added some general compare consume tests to ensure everything is generated ok.
|
|
Test failures look like failure to connect to services and missing spmi logs. |
I assume it can happen due to morph calling I think I've commented on this before, but I really would prefer if we just swapped the operands in the select node we create below (which seems more obviously correct to me, without having to rely on |
It's not quite that simple - you can't reverse a "single op" compare reverse into a non single op compare. Ordered Float NE -> Unordered Float EQ -> Ordered Float NE -> etc (always multi op) |
Gotcha, makes sense. So essentially we never see these multi-ops on ARM64.
I will leave it up to you. The handling you added in this PR in the backend looks innocent/correct enough. |
Ok, will leave as is then. |
|
Failures are #78290 (the CI run started right before the fix was merged) |
Are you sure? This PR isn't merged yet. |
|
I mean the CI failures in this PR are #78290 (which is fixed, but the fix for that issue didn't make it to this PR's CI run). |
Ah, sorry, Understood. |
Generating a FEQ or FNEU can generate an incorrect compare.
Firstly, the optimizer needs changing to ensure the NAN flag is correct.
Once that is fixed, it's not possible to generate a test to trigger the code in codegen (as there are no CIL beq.un or bne instructions). However, the codegen code was wrong.
Alternatively, given it's untested, it might be better to replace the codegen changes with some asserts?
I've added some general compare consume tests to ensure everything is generated ok.