Skip to content
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.

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

[JIT] Fixed improper peephole zero-extension removal when cdq/cwde instructions are involved #82733

Merged
merged 16 commits into from
Mar 5, 2023

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Feb 27, 2023

Resolves #82685

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 27, 2023
@ghost ghost assigned TIHan Feb 27, 2023
@ghost
Copy link

ghost commented Feb 27, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

Resolves #82685

Author: TIHan
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@@ -526,6 +526,16 @@ bool emitter::AreUpper32BitsZero(regNumber reg)
}
}

// This is a special case for cdq/cdqe/cwde.
// They always write to and sign-extend RAX.
Copy link
Member

Choose a reason for hiding this comment

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

notably: cwd is AX -> DX:AX
cdq is EAX -> EDX:EAX
cqo is RAX -> RDX:RAX

There are also a few other instructions that implicitly write specific outputs (typically edx and eax) or which take implicit inputs (often edx, eax, ecx, esi, or edi)

Wonder if they should be more generally modeled or tracked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They probably should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will need to check cdq for EDX.

Copy link
Member

@tannergooding tannergooding Feb 27, 2023

Choose a reason for hiding this comment

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

cmpxchg16b also can write RDX:RAX (and reads RCX:RBX)

div, idiv, imul, and mul writes RDX:RAX (and generally, but not always, reads the same)

Might be others that I'm forgetting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we emit cmpxchg16b or even cmpxchg8b AFAIK. But I did need to check for cmpxchg.

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if there is a good way to track such instructions or generalize the process with adequate asserts so we can catch the issues earlier? Tracking these issues with bad code gen are really hard to investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I can think of is to make it more table-driven, otherwise a function that asks the question should be adequate.

Given we are only doing these checks here, we probably could create a function that simply asks "Did the instruction write to this register?" and the result is either 'true' or 'false', and if 'true' we can also provide if it zero-extended or not.
I think I'm going to have to do this anyway for #82733 because I need to re-use the checks of whether or not a register was written to.

Copy link
Member

Choose a reason for hiding this comment

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

I am more worried if we miss an entry of one of the instructions that we support, we might have similar bug. Is there a manual that we can refer and put all those instructions in the table or whatever data structure we have and also post the link of the manual so someone looking at the code in future can know where it is coming from?

Copy link
Member

Choose a reason for hiding this comment

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

The Intel and AMD architecture manuals are the relevant resource.

The ones I listed are the ones that write two registers. There are others, like cmpxchg, which may write a single dedicated register.

It's likely just going to require someone going through the instructions and checking for cases that use a fixed/non-specified register (for input and/or output)

@TIHan TIHan changed the title [JIT] Fixed improper peephole zero-extension removal when cdq/cdqe/cwde instructions are involved [JIT] Fixed improper peephole zero-extension removal when cdq/cwde instructions are involved Feb 27, 2023
@TIHan
Copy link
Contributor Author

TIHan commented Feb 28, 2023

@dotnet/jit-contrib @kunalspathak this is ready

@TIHan
Copy link
Contributor Author

TIHan commented Feb 28, 2023

@TIHan
Copy link
Contributor Author

TIHan commented Mar 3, 2023

@dotnet/jit-contrib This is ready again. I introduced a function, emitIsInstrWritingToReg to contain all the logic to determine if a register was written to - pulled it from #82750

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JIT: Incorrectly eliminated zero-extending mov after cdqe
5 participants