Repository navigation
arm64: Avoid unused vector constant temps and prefer no-op casts - #136705
jonathandavies-arm wants to merge 1 commit into
Conversation
- Related to dotnet#136694 Match the vector immediate checks when building register requirements, so encodable constants do not reserve an unused integer temporary. Prefer the source register for non-overflowing int-to-int casts that can be elided when source and destination share a register. SuperPMI asm diffs against main (294b271), using Release JITs and six published Linux ARM64 collections, show 184 fewer generated code bytes. Of 60 changed compilation contexts, 45 became smaller and none grew.
github-actions
Bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
Bot
added
the
community-contribution
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
JulieLeeMSFT
left a comment
JulieLeeMSFT
left a comment
There was a problem hiding this comment.
LGTM.
The new canEncode check in LSRA matches CodeGen::genSetRegToConst: same is8 SIMD8/SIMD16 split and the same i32/i16/i8 MOVI checks.
The int temp is reserved only when codegen takes the memory-load fallback (emitIns_R_C).
The int -> int cast change is consistent with the existing long -> int handling.
Optionally, a shared helper for the MOVI check would remove the duplication, but that can be a follow-up.
| (ElementsAreSame(val.i32, is8 ? 2 : 4) && | ||
| emitter::emitIns_valid_imm_for_movi(val.i32[0], EA_4BYTE)) || | ||
| (ElementsAreSame(val.i16, is8 ? 4 : 8) && | ||
| emitter::emitIns_valid_imm_for_movi(val.i16[0], EA_2BYTE)) || | ||
| (ElementsAreSame(val.i8, is8 ? 8 : 16) && emitter::emitIns_valid_imm_for_movi(val.i8[0], EA_1BYTE)); |
There was a problem hiding this comment.
This is simply trying to find the smallest applicable broadcast, right?
Any reason we're manually checking ElementsAreSame rather than using vecCon->IsBroadcast(TYP_INT), then repeating for TYP_SHORT and TYP_BYTE?
Are we maybe missing a IsValidConstForMovImm(GenTreeVecCon*) helper and utilizing it more broadly?
There was a problem hiding this comment.
Yes we are trying to find an encodable broadcast, trying 32 bit elements, then 16 then 8. IsBroadcast looks suitable for SIMD8 and SIMD16.
For SIMD12, it checks only the 12 logical bytes, while codegen checks all 16 stored bytes before choosing MOVI. Using it directly for SIMD12 could make LSRA omit a temp that codegen needs for the memory-load path. Would you keep an explicit 16-byte check for SIMD12?
simd16_t val = vecCon->gtSimd16Val;
const bool is12 = vecCon->TypeIs(TYP_SIMD12);
// Codegen checks all 16 bytes for SIMD12, while IsBroadcast checks only 12.
bool canEncode =
((is12 ? ElementsAreSame(val.i32, 4) : vecCon->IsBroadcast(TYP_INT)) &&
emitter::emitIns_valid_imm_for_movi(val.i32[0], EA_4BYTE)) ||
((is12 ? ElementsAreSame(val.i16, 8) : vecCon->IsBroadcast(TYP_SHORT)) &&
emitter::emitIns_valid_imm_for_movi(val.i16[0], EA_2BYTE)) ||
((is12 ? ElementsAreSame(val.i8, 16) : vecCon->IsBroadcast(TYP_BYTE)) &&
emitter::emitIns_valid_imm_for_movi(val.i8[0], EA_1BYTE));
There was a problem hiding this comment.
I'd expect that TYP_SIMD12 is already optimizing itself to self-broadcast and adjusting earlier if appropriate so that codegen does the "right thing". So I would expect we just use IsBroadcast everywhere here and tweak things later if we find suboptimal TYP_SIMD12 handling
Match the vector immediate checks when building register requirements, so encodable constants do not reserve an unused integer temporary. Prefer the source register for non-overflowing int-to-int casts that can be elided when source and destination share a register.
SuperPMI asm diffs against main (294b271), using Release JITs and six published Linux ARM64 collections, show 184 fewer generated code bytes. Of 60 changed compilation contexts, 45 became smaller and none grew.