JIT: Add some more constant folding in lowering #113301
Conversation
With recent work to expand returned promoted locals into `FIELD_LIST` the only "whole references" of promoted locals we should see is when stored from a multi-reg node. This is the only knowledge the backend should need for correctness purposes, so introduce a bit to track this property, and switch the backend to check this instead. The existing `lvIsMultiRegRet` is essentially this + whether the local is returned. We should be able to remove this, but it is currently used for some heuristics in old promotion, so keep it around for now.
Add folding for shifts and certain binops that are now getting produced
late due to returned `FIELD_LIST` nodes.
win-arm64 example:
```csharp
[MethodImpl(MethodImplOptions.NoInlining)]
static ValueTask<byte> Foo()
{
return new ValueTask<byte>(123);
}
```
```diff
G_M17084_IG02: ;; offset=0x0008
mov x0, xzr
- mov w1, #1
- mov w2, wzr
- mov w3, dotnet#123
- orr w2, w2, w3, LSL dotnet#16
- orr w1, w2, w1, LSL dotnet#24
- ;; size=24 bbWeight=1 PerfScore 4.00
+ mov w1, #0x17B0000
+ ;; size=8 bbWeight=1 PerfScore 1.00
```
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/lower.cpp
Outdated
|
|
||
| if (op1->IsIntegralConst() && op2->IsIntegralConst() && | ||
| op1->AsIntConCommon()->ImmedValCanBeFolded(comp, node->gtOper) && | ||
| op2->AsIntConCommon()->ImmedValCanBeFolded(comp, node->gtOper)) |
There was a problem hiding this comment.
Do we need these checks? I'd expect gtFoldExprConst to check them
There was a problem hiding this comment.
Good point, looks like it does -- removed
There was a problem hiding this comment.
(I kept the IsIntegralConst checks because I want some control over when we call this "normally used on HIR" API)
src/coreclr/jit/lower.cpp
Outdated
| } | ||
|
|
||
| op1->SetUnusedValue(); | ||
| op2->SetUnusedValue(); |
There was a problem hiding this comment.
Do we often just mark nodes as unused without deleting them?
There was a problem hiding this comment.
Seems better to just remove them given they are simple constants that won't ever have side effects. Changed to that.
| return true; | ||
| } | ||
|
|
||
| if (node->OperIs(GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR, GT_OR, GT_XOR) && |
There was a problem hiding this comment.
sounds like what gtFoldExprSpecial does
There was a problem hiding this comment.
I presume the problem with calling gtFold* apis in Lower is that they must not create new nodes (other than what they return)
There was a problem hiding this comment.
Yes, I am trying to use the lowest level API I can for this to not run into issues calling this from lower... There's probably a better way to factor this folding to make that clear.
There was a problem hiding this comment.
Maybe it would be better to have something like
bool gtFoldExprValue32(GenTree* node, int32_t* result);
bool gtFoldExprValue64(GenTree* node, int64_t* result);and then call that from both gtFoldExprConst and lowering.
There was a problem hiding this comment.
I don't think I'm going to try changing this here. We should probably refactor gtFold* APIs to give it some idea of what kind of IR it is allowed to create instead, and then use it more directly from lowering.
Add folding for shifts and certain binops that are now getting produced late due to returned
FIELD_LISTnodes.win-arm64 example:
G_M17084_IG02: ;; offset=0x0008 mov x0, xzr - mov w1, #1 - mov w2, wzr - mov w3, #123 - orr w2, w2, w3, LSL #16 - orr w1, w2, w1, LSL #24 - ;; size=24 bbWeight=1 PerfScore 4.00 + mov w1, #0x17B0000 + ;; size=8 bbWeight=1 PerfScore 1.00Based on #113294