Eliminate redundant test instruction #53214
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@dotnet/jit-contrib |
| case INS_shl: | ||
| case INS_shr: | ||
| case INS_sar: | ||
| return false; |
There was a problem hiding this comment.
I think this might require some careful considerations.
For the purposes of this PR, this is fine and is simply used as a "did the previous instruction set the flag and therefore a test can be skipped" check.
However, for future improvements we may consider more advanced analysis such as:
if (!IsFlagsModified(prevInstruction))
{
// can depend on prevInstruction - 1 state being valid
}
Then, this will become problematic. The latter happens in Clang/MSVC in various places and are one of the reasons that mulx, rorx, sarx, shlx, and shrx are exposed on modern hardware.
There was a problem hiding this comment.
Perhaps we can return some ternary state here representing "yes", "no", and "it depends"?
There was a problem hiding this comment.
Today, we only track prevInstruction (emitLastIns). The way I read your comment, we will need to have access to last couple of instructions (if not more) to make sure the state is valid?
There was a problem hiding this comment.
Right, it's not something I think we need to be concerned with "today", but rather something I could see being problematic in the future for certain optimizations.
The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.
There was a problem hiding this comment.
The method implies boolean state but in practice its slightly more nuanced and one of the answers is "maybe", so it may end up confusing callers.
Exactly - what are callers supposed to do with "maybe". So for now, I will leave it as-is.
There was a problem hiding this comment.
I think that is contextual. In the case of this PR, you care "did the last instruction definitely set the flag I care about" and so you would treat maybe as false.
In other cases, optimizations may care "did the last instruction definitely not set the flag I care about" and so they would treat it as true.
There was a problem hiding this comment.
Renamed the method to IsFlagsAlwaysModified.
|
I think the changes overall look good and AFAICT are tracking the right state for the flags. |
|
@BruceForstall or @AndyAyersMS, do you mind taking a look? |
src/coreclr/jit/emitxarch.cpp
Outdated
| } | ||
|
|
||
| //------------------------------------------------------------------------ | ||
| // IsFlagsAlwaysModified: check if the instruction always modifies the flags. |
There was a problem hiding this comment.
| // IsFlagsAlwaysModified: check if the instruction always modifies the flags. | |
| // AreFlagsAlwaysModified: check if the instruction always modifies the flags. |
I am a bit confused on what this is supposed to check. Does it mean: return true if there is at least one flag bit that is always modified by this instruction?
Seems odd we would ask that question instead of asking whether some specific set of flags is always modified.
There was a problem hiding this comment.
For shift instructions, if a shift-amount is 0, it keeps the flags unmodified. For them, we shouldn't rely on last instruction, but the flags might have set by one of the previous instructions. As such, this method returns false saying that these instructions (under certain circumstances) guaranteed to not modify flags and the caller shouldn't rely on last instruction to eliminate test. For remaining instructions, they modify some flags and can eliminate test safely. I will add more explicit comment.
See my comment #53214 (comment) when I was not handling this case.
ghost
locked as resolved and limited conversation to collaborators
Eliminate
testinstruction if previous instruction writes toZFflag. Thanks to @tannergooding for updating the instruction table with flags that it reads/writes. I use that information in the code added by @nathan-moorein #38586. It covers other remaining cases for
JE/JNE, specially around bit operation instructions. In future, we would expand to cover other flags/Jcc instructions.Also included, minimal natvis to display instructions.
Contributes to #53053.
Below is the asmdiff summary. 1-2 method regression comes because of loop alignment adjustment.
Unix x64 benchmarks
Detail diffs
Unix x64 libraries
Detail diffs
Windows x64 asp.net
Detail diffs
Windows x64 benchmarks
Detail diffs
Windows x64 libraries
Detail diffs