Arm64: Pass the small size accurately to emitIns_valid_imm_for_ldst_offset #91741
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPass the correct size of instruction to determine if the immediate will fit in the instruction or not. Fixes: #68536
|
|
@dotnet/jit-contrib |
| case INS_ldrsh: | ||
| case INS_ldrsw: | ||
| case INS_ldrb: | ||
| immFitsInIns = emitter::emitIns_valid_imm_for_ldst_offset(imm, EA_1BYTE); |
There was a problem hiding this comment.
Why is size not already EA_1BYTE in this case?
There was a problem hiding this comment.
It is passed by the caller genEnregisterOSRArgsAndLocals() using emitActualTypeSize(lclTyp), which for TYP_BYTE returns EA_4BYTE.
There was a problem hiding this comment.
It seems like the bug is in genEnregisterOSRArgsAndLocals() -- it appears lclTyp is TYP_BYTE so it is asking for a ldrb but it passes the actual size (4) instead of size 1. It looks like lclTyp can only be small for OSR local struct fields. Maybe it should use const emitAttr size = emitTypeSize(lclTyp); instead of using emitActualTypeSize()? @AndyAyersMS ?
There was a problem hiding this comment.
Yes, I think we should be using emitTypeSize in genEnregisterOSRArgsAndLocals.
There was a problem hiding this comment.
It looks like lclTyp can only be small for OSR local struct fields.
Why is the reasoning behind it?
emitTypeSize
I never understood the difference between emitTypeSize and emitActualTypeSize(). In which scenarios should we use one over other?
There was a problem hiding this comment.
Turns out that for emit_R_R_I() format, if we are sign extending, we expect the size to be valid general size i.e. 4 bytes or 8 bytes, so changing to emitTypeSize() hits an assert. I will follow up offline with @BruceForstall .
|
@BruceForstall PTAL. |
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6180624899 |
ghost
locked as resolved and limited conversation to collaborators
3 participants
Pass the correct size of instruction to determine if the immediate will fit in the instruction or not.
Fixes: #68536