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

update the location of assert for REG_ZR check #112294

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kunalspathak
Copy link
Member

@kunalspathak kunalspathak commented Feb 7, 2025

This probably got exposed with #111451 because that makes more usages of SP than FP. While loading or storing from stack with an offset, we already encode SP -> ZR, but if we have to produce the offset using reserved register, we perform add or sub to populate it. The caller already encodes REG_SP as REG_ZR, but in add/sub, we were expecting the reg2 to be SP or GPR, but later we will encode it to ZR anyway. So, I have just moved the assert to take into account of that.

Fixes: #112278

@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 7, 2025
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib @amanasifkhalid

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@@ -5875,6 +5875,9 @@ void emitter::emitIns_R_R_I(instruction ins,
return;
}

// reg2 can be alread encoded to zero
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// reg2 can be alread encoded to zero
// reg2 can be already encoded to zero

Copy link
Member Author

Choose a reason for hiding this comment

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

ok to fix in next batch? don't want to run CI again.

Copy link
Member

Choose a reason for hiding this comment

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

Sure

@jakobbotsch
Copy link
Member

but if we have to produce the offset using reserved register, we perform add or sub to populate it

Shouldn't the code that produces the add/sub be doing encodingZRtoSP instead when encoding it? I think it would be better to keep the requirement that reg2 is never ZR, since it's not a supported possible instruction.

@kunalspathak
Copy link
Member Author

but if we have to produce the offset using reserved register, we perform add or sub to populate it

Shouldn't the code that produces the add/sub be doing encodingZRtoSP instead when encoding it? I think it would be better to keep the requirement that reg2 is never ZR, since it's not a supported possible instruction.

Yeah, the emitIns_R_R_I() is a very common method and was not sure if that's the right thing to do. But this is happening in scenario where we populate base address for predicate register, so let me point fix those 2 places only.

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

Choose a reason for hiding this comment

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

Agreed that fixing the caller seems better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 'isGeneralRegisterOrSP(reg2)' in SVE tests
3 participants