Uh oh!
There was an error while loading. Please reload this page.
Contain memory operands under casts - #72719
Conversation
ghost
commented
Jul 23, 2022
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFold the sign/zero-extension into an appropriate load. The change implements full support on XARCH, partial support on ARM/64, and punts LA. A follow-up issue will be filed to track the remaining work. The ARM/64 commit contains some notes on what is required for said support on our load/store architectures. The diffs are nice and simple: - ldr lr, [sp+0x34] // [V07 arg7]- uxtb lr, lr+ ldrb lr, [sp+0x34] // [V07 arg7]
|
d3d8af4 to
6ffddaeCompareSingleAccretion
commented
Jul 24, 2022
@dotnet/jit-contrib |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
SingleAccretion
commented
Jul 28, 2022
I think it would be good to run the fuzzers and [libraries] stress on this change. |
EgorBo
commented
Jul 28, 2022
/azp run Fuzzlyn, Antigen, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 6 pipeline(s). |
TODO: consider using a dedicated IND_EXT oper for ARM/ARM64 instead of containment. This would allow us to cleany handle all indirections. It would not mean we'd give up on the casts containment, as we'd still need to handle the "reg optional" case. IND_EXT will be much like an ordinary IND, but have a "source" and "target" types. The "target" type would always be int/long, while "source" could be of any integral type. This design would be a bit more natural, and nicely separable from casts. However, the main problem with the current state of things, apart from the fact codegen of indirections is tied strongly to "GenTreeIndir", is the fact that changing type of the load can invalidate LEA containment. One would think this is solvable with some tricks, like re-running containment analysis on an indirection after processing the cast, but ARM64 codegen doesn't support uncontained LEAs in some cases. A possible solution to that problem is uncontaining the whole address tree. That would be messy, but ought to work. An additional complication is that these trees can contain a lot of contained operands as part of ADDEX and BFIZ, so what would have to be done first is the making of these into proper EXOPs. In any case, this is all future work.
In CAST<short>(IND<byte>(...)), "m_extendSrcSize" must be "1". Modulo the above, stress runs looked clean.
0da2d79 to
7ec2a8aCompare
jakobbotsch
left a comment
There was a problem hiding this comment.
It's curious to me that there are so many x86 improvements. Do we not already removed most of these unnecessary casts there by retyping loads in morph?
SingleAccretion
commented
Aug 23, 2022
I think most of these come from normalize-on-load stack parameters, which we do not retype. |
jakobbotsch
commented
Aug 23, 2022
Doesn't morph introduce this IR shape itself in |
SingleAccretion
commented
Aug 23, 2022
It of course does, and it is, but it is necessary for in-register parameters (and on-stack ones as well, though that should be relatively easy to fix by using extending loads in Morph's logic there is questionable in more than one way. If we know the local will be DNER at that point, we could retype it to its small type instead of introducing a cast. However, this turns out to sometimes not be a win because we don't CSE locals, but do CSE casts. |
jakobbotsch
commented
Aug 24, 2022
Makes sense about CSE. The way Anyway, thanks for the contribution as usual. |
Improvements on |
Possible regressions: |
Recognize legal patterns in lowering and then fold the sign/zero-extension into an appropriate load at codegen time.
The change implements full support on XARCH, partial support on ARM/64, and punts LA. A follow-up issue will be filed to track the remaining work. The ARM/64 commit contains some notes on what is required for said support on our load/store architectures.
The diffs are nice and simple:
Regressions are RA/alignment.