-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Fix issue #32466 #32804
Fix issue #32466 #32804
Conversation
Fixes outerloop leg: R2R Windows_NT x86 Checked no_tiered_compilation @ Windows.10.Amd64.Open
@dotnet/jit-contrib |
src/coreclr/src/jit/emitxarch.cpp
Outdated
@@ -2835,7 +2835,13 @@ void emitter::emitHandleMemOp(GenTreeIndir* indir, instrDesc* id, insFormat fmt, | |||
|
|||
// Either not generating relocatable code, or addr must be an icon handle, or the | |||
// constant is zero (which we won't generate a relocation for). | |||
assert(!emitComp->opts.compReloc || memBase->IsIconHandle() || memBase->IsIntegralConst(0)); | |||
// | |||
// On x86 we can have an indirection of a non-handle int constant without a reloc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: With this change, it's not entirely clear that this comment is an extension of the previous comment. Also, while you're here, the previous comment isn't clear about whether this is a statement (i.e. restating the condition of the assert), or describing a condition, e.g. the if statement below).
Could you reword this as a complete statement? Perhaps:
// If we reach here, either:
// - we are not generating relocatable code,
// - the base address is a handle for an integer constant,
// - the base address is a constant zero (which doesn't require a reloc), or
// - the base address is a constant that fits into the memory instruction (this can happen on x86).
// This last case is captured in the FitsInAddrBase method which is used by Lowering to determine that it can
// be contained.
Of course, please correct if the above is not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK can change the comment.
I didn't like the existing comment, It isn't clear to me why we are testing a different set of invariants here than we when originally set the contained flag. And actually all 31 bit constants don't require a relocation, not just zero (for both x84 and x86)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will use your comment and omit this part (which doesn't require a reloc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I changed it to:
// If we reach here, either:
// - we are not generating relocatable code, (typically the non-AOT JIT case)
// - the base address is a handle represented by an integer constant,
// - the base address is a constant zero, or
// - the base address is a constant that fits into the memory instruction (this can happen on x86).
// This last case is captured in the FitsInAddrBase method which is used by Lowering to determine that it can
// be contained.
//
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The last update to this assert was this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
Fixes outerloop leg: R2R Windows_NT x86 Checked no_tiered_compilation @ Windows.10.Amd64.Open
Fixes issue #32466