Enable more constant foldings for switch ops using assertions#113998
Enable more constant foldings for switch ops using assertions#113998EgorBo merged 6 commits intodotnet:mainfrom
Conversation
e287095 to
c7882ca
Compare
|
@EgorBot -amd using BenchmarkDotNet.Attributes;
public class Bench
{
static string[] _strings = ["1", "10", "100", "1000", "10000", "100000", "1000000"];
[Benchmark]
public int Sum()
{
int sum = 0;
foreach (string str in _strings)
{
sum += str switch
{
"1" => 1,
"10" => 10,
"100" => 100,
"1000" => 1000,
"10000" => 10000,
"100000" => 100000,
"1000000" => 1000000,
_ => -1
};
}
return sum;
}
} |
|
PTAL @AndyAyersMS @dotnet/jit-contrib Diffs I decided to implement the assertion using a NOP node in all branches as the simplest impl, but as a follow up, I want to remove this "single assertion per tree" for Global Prop and write directly into My first impl was writing into bbAssertionIn/bbAssertionGen but it generated less diffs, presumably because we already ran |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Looks good overall. Left you a few small things to consider.
| // TODO-Cleanup: We shouldn't attach assertions to nodes in Global Assertion Prop. | ||
| // It limits the ability to create multiple assertions for the same node. | ||
| GenTree* tree = gtNewNothingNode(); | ||
| fgInsertStmtAtBeg(target, fgNewStmtFromTree(tree)); |
There was a problem hiding this comment.
You should assert there are no PHI nodes in this block (there shouldn't be any since the block has a single pred edge).
|
|
||
| if (containsSwitches) | ||
| { | ||
| for (BasicBlock* const block : Blocks()) |
There was a problem hiding this comment.
There are unlikely to be many switches so maybe keep track of them in the initial walk?
| // We can only make "X == caseValue" assertions for blocks with a single edge from the switch. | ||
| BasicBlock* target = jumpTable[jmpTargetIdx]->getDestinationBlock(); | ||
| if ((target->GetUniquePred(this) != switchBb) || (fgGetPredForBlock(target, switchBb)->getDupCount() > 1) || | ||
| !BasicBlock::sameEHRegion(target, switchBb)) |
There was a problem hiding this comment.
Do we need to bail if not same EH region? If the switch arm is a try entry still seems fine to add the assertion.
There was a problem hiding this comment.
hm.. I think you're right, relaxed
|
/ba-g "azurelinux.3 timeouts" |
Closes #113992
This PR creates global assertions for switch targets.
An example from System.Text.Json:
Codegen diff (Main vs PR): https://www.diffchecker.com/cxZY6tn3/ (still not perfect, though).
Another use-case is
switchover string literals. Roslyn emits a trie-like algorithm for them where we do switch over string's length and end up comparing length inside all cases:Codegen diff (Main vs PR): https://www.diffchecker.com/CeHgRf8I/