[JIT] Allow side-effects for the first test in switch recognition #106988
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@EgorBot -intel -keep --envvars DOTNET_JitDisasm:BitTest DOTNET_JitStdOutFile:jitdisasm.asm using System;
using System.Runtime.CompilerServices;
using BenchmarkDotNet.Attributes;
public class Bencha
{
string test = "qwertyuiopasdfghjjklzxcvbnmroigjeroijgiroejg";
[Benchmark]
public int Bench()
{
int count = 0;
foreach (var ch in test)
{
if (BitTest(ch))
count++;
}
return count;
}
[MethodImpl(MethodImplOptions.NoInlining)]
static bool BitTest(char ch)
{
if (ch == 'a' || ch == 'c' || ch == 'n' || ch == 'h' || ch == 'z')
return true;
return false;
}
} |
Benchmark results on Intel
|
| @@ -89,7 +91,15 @@ bool IsConstantTestCondBlock(const BasicBlock* block, | |||
| if ((op1->IsCnsIntOrI() && !op1->IsIconHandle()) ^ (op2->IsCnsIntOrI() && !op2->IsIconHandle())) | |||
| { | |||
| // TODO: relax this to support any side-effect free expression | |||
There was a problem hiding this comment.
Is there any more side effect to support other than COMMA? If not, please remove the TODO comment becasue this PR will support COMMA side-effect.
|
Please backport to .NET 9 RC2. |
| @@ -324,7 +332,7 @@ bool Compiler::optSwitchConvert( | |||
| BasicBlock* blockIfTrue = nullptr; | |||
| BasicBlock* blockIfFalse = nullptr; | |||
| bool isReversed = false; | |||
| const bool isTest = IsConstantTestCondBlock(lastBlock, &blockIfTrue, &blockIfFalse, &isReversed); | |||
| const bool isTest = IsConstantTestCondBlock(lastBlock, false, &blockIfTrue, &blockIfFalse, &isReversed); | |||
There was a problem hiding this comment.
Why do we clone nodeToTest below? Initially it seemed like we were cloning the side effects too, but it looks like we discard the old version, so no cloning would be necessary.
There was a problem hiding this comment.
I think this clone was an artifact from the initial impl, I have removed it in the latest commit.
There was a problem hiding this comment.
I think it looks fine.. may want to double check that we aren't cloning any side effect.
It's odd to me that this transformation depends on lexical order of the basic blocks. That seems unnecessary and something we have been trying to move away from. (Edit: Opened #107076 for this)
Fixed.
@JulieLeeMSFT are you sure this needs a backport? the perf impact seems to be minimal (around 1-2% in my benchmarks). or this needed simply for marketing purposes? cc @stephentoub |
Codegen diff:
; Assembly listing for method Proga:Test(ushort) (FullOpts) push rbx sub rsp, 32 movzx rbx, cx - cmp ebx, 13 ;; this "ch == '\r'" is supposed to be part of the bit-test below - je SHORT G_M18843_IG05 cmp ebx, 32 ja SHORT G_M18843_IG06 - mov eax, 0xFFFFF9FF + mov eax, 0xFFFFD9FF bt rax, rbx jb SHORT G_M18843_IG06 G_M18843_IG05: mov rcx, 0x2127BF35718 ; 'special symbol' call [System.Console:WriteLine(System.String)] G_M18843_IG06: nop add rsp, 32 pop rbx retJIT IR before the switch recognition:
We used to give up on
BB01due toCOMMA, but in fact we shouldn't since we preserve the first block for the resultingswitch.Alternative fix: run "remove all commas" phase 🙂
SplitTreesRemoveCommas