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: Fix overzealous assert in CodeGen::genSSE42Intrinsic #105833

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

amanasifkhalid
Copy link
Member

@amanasifkhalid amanasifkhalid commented Aug 1, 2024

Fixes #105821.

@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 Aug 1, 2024
@amanasifkhalid
Copy link
Member Author

cc @dotnet/jit-contrib

@jakobbotsch
Copy link
Member

jakobbotsch commented Aug 1, 2024

Isn't the assert fine? If it triggers it should mean problems. We would override the "data" (in op2) with the current CRC value (in op1Reg), since targetReg and op2Reg coincide.

What are the registers in this case?

@jakobbotsch
Copy link
Member

I think the assert should instead be:

assert(!op2->isUsedFromReg() || (op2->GetRegNum() != targetReg) || (op1Reg == targetReg));

@tannergooding
Copy link
Member

!op2->isUsedFromReg()

Are we getting register assignments when we aren't used from reg?

@jakobbotsch
Copy link
Member

jakobbotsch commented Aug 1, 2024

Evidently so. In this case op2 is used from a spill temp, so it needs a register to compute the def into. However, it would probably be possible for codegen to unset the register assignment as part of processing the def.

@tannergooding
Copy link
Member

That definitely seems like a safer and more robust long term solution; but probably not something we want for .NET 9

I think your proposed assert handles it well enough for the short term fix. We do indeed need to ensure that the mov tgt, op1 isn't overwriting op2, as that would produce incorrect results (and likely mean we forgot a delayFree somewhere).

@amanasifkhalid
Copy link
Member Author

What are the registers in this case?
targetReg and op2->GetRegNum() are rax, op1Reg is rdx.

Thanks for the proposed assert, and for opening an issue. I think you're right that we didn't unset the register assignment for op2 somewhere -- the updated assert works.

@amanasifkhalid amanasifkhalid changed the title JIT: Remove overzealous assert in CodeGen::genSSE42Intrinsic JIT: Fix overzealous assert in CodeGen::genSSE42Intrinsic Aug 1, 2024
@amanasifkhalid amanasifkhalid merged commit f4c4399 into dotnet:main Aug 2, 2024
121 checks passed
@amanasifkhalid amanasifkhalid deleted the crc32-bad-assert branch August 2, 2024 15:10
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2024
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: Assertion failed '(op2->GetRegNum() != targetReg) || (op1Reg == targetReg)' during 'Generate code'
4 participants