Fixes GT_MOD codegen and three other issues for ARM64 #3390
Conversation
| genJumpToThrowHlpBlk(EJ_jmp, SCK_DIV_BY_ZERO); | ||
|
|
||
| // We still need to produce a register result | ||
| emit->emitIns_R_R(INS_mov, size, targetReg, REG_ZR); |
There was a problem hiding this comment.
This is dead code and follows after jmp instruction. If it is required to set targetReg to zero, shouldn't this be before genJumpToThrowHlpBlk()?
There was a problem hiding this comment.
For correctness we have to call genProduceReg(treeNode);
That method handles spilling and marking something as a GC or non GC pointer.
To me it make sense to put an updated value into the destination register rather than leave a possibly uninitialized value there. The extra code size of one instruction for the case of an explicit division by the constant zero is a negligible cost to prevent a possible GC pointer information leak.
There was a problem hiding this comment.
Div operation never produces a gc-ref. Besides both code emitted by emitins_R_R() and the code produced by genProduceReg() both are dead code and will never get executed. From that perspective setting targetReg to zero isn't serving any purpose.
I understand genProduceReg() call is still required for updating the life of targetReg but i don't see why we need to emit an instruction to set targetReg to zero. That was my point
There was a problem hiding this comment.
OK, after talking with you I will remove the mov targetReg, ZR instruction
There was a problem hiding this comment.
On second thought, are we sure that codegen needs to handle divisor being a constant integer zero? Isn't morph ensuring div-by-zero gets morphed into a 'Throw' so that the basic block will be marked to end with a throw and that might lead to dead code elimination?
If front-end ensures the invariant that backend never sees div-by-zero constant, we just need to add an assert here.
|
Looks good. |
|
LGTM |
|
LGTM |
Fix Issue 3263 - Incorrect codegen for rem.un - Unisgned Remainder Fix Issue 3317 - Improve the codegen for divide so that we omit checks when we have a constant divisor Fix Issue 3326 - assert EA_4BYTE or EA_8BYTE Fix an assert when using alloca with allocation between 65 and 4095 bytes Also fixed the name for GT_UDIV and GT_UDIV to contain a "un-" prefix. Added 10 additional passing tests with tag REL_PASS
Fixes GT_MOD codegen and three other issues for ARM64
Fix Issue 3263 - Incorrect codegen for rem.un - Unisgned Remainder
Fix Issue 3317 - Improve the codegen for divide so that we omit checks when we have a constant divisor
Fix Issue 3326 - assert EA_4BYTE or EA_8BYTE
Fix an assert when using alloca with allocation between 65 and 4095 bytes
Also fixed the name for GT_UDIV and GT_UDIV to contain a "un-" prefix.