-
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 build break on osx/arm64 Debug #102631
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/regset.cpp
Outdated
@@ -44,6 +44,34 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |||
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |||
*/ | |||
|
|||
#ifdef SWIFT_SUPPORT | |||
regMaskTP rsAllCalleeSavedMask; | |||
regMaskTP rsIntCalleeSavedMask; |
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.
Do you think it would be cleaner to initialize these here instead of in (Never mind, I realized we want the constructor to reset these values between methods compiled) Also, it might be nice to add a comment explaining these aren't RegSet
's constructor below?const
or constexpr
because rsClearRegsModified
(and only this method) may modify them in Swift interop scenarios.
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.
Do you think it would be cleaner to initialize these here instead of in RegSet's constructor below?
sure.
constexpr
Was there a specific TP improvement we saw by marking them constexpr
? I am inclined to just make them and it is just cleaner.
regMaskTP rsAllCalleeSavedMask = RBM_CALLEE_SAVED;
regMaskTP rsIntCalleeSavedMask = RBM_INT_CALLEE_SAVED;
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.
Will check if there is any TP impact with this change.
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.
Was there a specific TP improvement we saw by marking them constexpr?
This was part of a bigger change, so I'm not sure what the TP impact is of changing this. I'm curious to see as well. If there is no impact, then maybe we should move them back into the RegSet
class.
Thanks @kunalspathak! Build on osx-arm64 is succeeded. 👍 |
* fix build break on osx/arm64 Debug * remove constexpr * Revert "remove constexpr" This reverts commit e67ca1a. * Revert "fix build break on osx/arm64 Debug" This reverts commit dc5ad92. * remove static constexpr --------- Co-authored-by: Kunal Pathak <[email protected]>
Fixes: #102618 and the errors reported in
#102297 (comment)