Fix AreFlagsSetToZeroCmp to not consider unsupported formats #85714
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis resolves #85637
|
src/coreclr/jit/emitxarch.cpp
Outdated
| if (!id->idIsReg1Write() || (id->idReg1() != reg)) | ||
| { | ||
| // Don't consider instructions which didn't write a register | ||
| return false; | ||
| } | ||
|
|
||
| if (id->idHasMemWrite() || id->idIsReg2Write()) | ||
| { | ||
| // Don't consider instructions which also wrote a mem location or second register | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is more conservative than it needs to be but brings it back inline with what was previously being checked:
switch (fmt)
{
case IF_RWR_CNS:
case IF_RRW_CNS:
case IF_RRW_SHF:
case IF_RWR_RRD:
case IF_RRW_RRD:
case IF_RWR_MRD:
case IF_RWR_SRD:
case IF_RRW_SRD:
case IF_RWR_ARD:
case IF_RRW_ARD:
case IF_RWR:
case IF_RRD:
case IF_RRW:
case IF_RWR_RRD_RRD:
case IF_RWR_RRD_MRD:
case IF_RWR_RRD_ARD:
case IF_RWR_RRD_SRD:
break;
default:
return false;
}The general issue was that the check missed cases like xadd which wrote two registers or a mem location + a register and while that isn't "incorrect" to do, handling it would require more checks to ensure the correct register write impacted the flags.
I opted for the faster/more conservative fix and will log an issue tracking adding the correctly handling for cases like xadd
|
There's also |
There were only a handful of places that had big switch table removals; the other places just had additions to the existing switches to cover the "missing" formats. Of the places that removed switch tables, That leaves |
| if (!id->idIsReg1Write() || (id->idReg1() != reg)) | ||
| { | ||
| // Don't consider instructions which didn't write a register | ||
| return false; | ||
| } | ||
|
|
||
| if (id->idHasMemWrite() || id->idIsReg2Write()) | ||
| { | ||
| // Don't consider instructions which also wrote a mem location or second register | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Similarly, the previous switch was:
switch (fmt)
{
case IF_RWR_CNS:
case IF_RRW_CNS:
case IF_RRW_SHF:
case IF_RWR_RRD:
case IF_RRW_RRD:
case IF_RWR_MRD:
case IF_RWR_SRD:
case IF_RRW_SRD:
case IF_RWR_ARD:
case IF_RRW_ARD:
case IF_RWR:
case IF_RRD:
case IF_RRW:
break;
default:
return false;
}
This resolves #85637