Skip to content

Extend the "skip-test" optimization to all instructions that modify the #120876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+17 −9
Open

Extend the "skip-test" optimization to all instructions that modify the #120876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions src/coreclr/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,19 +758,29 @@ bool emitter::DoesWriteSignFlag(instruction ins)
}

//------------------------------------------------------------------------
// DoesResetOverflowAndCarryFlags: check if the instruction resets the
// OF and CF flag to 0.
// DoesModifySameFlagsAsTESTInst: check if the instruction modifies
// the same flags as a "TEST" instruction, which allows some optims
Copy link
Member

@kg kg Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we provide more detail on what it allows, if it's not too verbose?

Copy link
Author

@Lotendan Lotendan Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kg thans for the review, very appreciated 🙂
Yes sure I can add more info and point out that this allows to potentially skip a test instruction.

//
// Arguments:
// ins - instruction to test
//
// Return Value:
// true if instruction resets the OF and CF flag, false otherwise.
// true if instruction modifies the same flags as "TEST" instruction
//
bool emitter::DoesResetOverflowAndCarryFlags(instruction ins)
// Note:
// "TEST" instruction:
// - Resets OF and CF flags to zero
// - Sets SF, ZF, PF flags from the result
// - Leaves AF undefined
Copy link
Author

@Lotendan Lotendan Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure here whether it makes sense to add a condition on AF flag as well

//
bool emitter::DoesModifySameFlagsAsTESTInst(instruction ins)
{
insFlags flags = CodeGenInterface::instInfo[ins];
return (flags & (Resets_OF | Resets_CF)) == (Resets_OF | Resets_CF);
return (flags & (Resets_OF | Writes_OF)) != 0 &&
(flags & (Resets_CF | Writes_CF)) != 0 &&
(flags & (Resets_SF | Writes_SF)) != 0 &&
(flags & (Resets_ZF | Writes_ZF)) != 0 &&
(flags & (Resets_PF | Writes_PF)) != 0;
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -1457,9 +1467,7 @@ bool emitter::AreFlagsSetToZeroCmp(regNumber reg, emitAttr opSize, GenCondition

// Certain instruction like and, or and xor modifies exactly same flags
// as "test" instruction.
// They reset OF and CF to 0 and modifies SF, ZF and PF.
if (DoesResetOverflowAndCarryFlags(lastIns) && DoesWriteSignFlag(lastIns) && DoesWriteZeroFlag(lastIns) &&
DoesWriteParityFlag(lastIns))
if (DoesModifySameFlagsAsTESTInst(lastIns))
Copy link
Member

@kg kg Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This previously checked specifically that the OF and CF were reset but now it's sufficient to write them instead of reset them, which means they might be nonzero. But this method is named AreFlagsSetToZeroCmp, so this check becomes misleading. I think OF and CF should be restricted to Resets.

Copy link
Author

@Lotendan Lotendan Nov 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too sure what you expect here or how I could improve the code to make it more readable. Perhaps renaming DoesModify into ˋDoesUpdateˋ? Would that help? Or do you have any other suggestion/insight?
The previous code used to check for Write and not just Reset just as well.
Basically if an instruction modifies the same flags as a test instruction (be it reset or write) then we can skip a test instruction indeed.

Copy link
Member

@kg kg Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code was (flags & (Resets_OF | Resets_CF)) == (Resets_OF | Resets_CF) but now it is Resets_OF | Writes_OF and Resets_CF | Writes_CF, which is a difference. I'm not sure if it's bad though.

{
return id->idOpSize() == opSize;
}
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/emitxarch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,7 @@ static bool HasRegularWideImmediateForm(instruction ins);
static bool DoesWriteZeroFlag(instruction ins);
static bool DoesWriteParityFlag(instruction ins);
static bool DoesWriteSignFlag(instruction ins);
static bool DoesResetOverflowAndCarryFlags(instruction ins);
static bool DoesModifySameFlagsAsTESTInst(instruction ins);
bool IsFlagsAlwaysModified(instrDesc* id);
static bool IsRexW0Instruction(instruction ins);
static bool IsRexW1Instruction(instruction ins);
Expand Down
Loading