-
Notifications
You must be signed in to change notification settings - Fork 10.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
[X86]RMW instrs not handled in pre-RA-sched=fast #67281
Conversation
// unfolding an x86 RMW operation results in store, dec, load which | ||
// can't be handled here so quit | ||
if (NewNodes.size() == 3) | ||
return nullptr; | ||
|
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.
Should this put in X86 specific function? Otherwise, it may affect other targets.
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.
patch in ScheduleDAGRRList.cpp was fixed in this way, then maybe need to add 1 function to TargetInstrInfo if that.
But I may raise another PR which could disable unfold for X86 RMW instrs, I just find it could also fix it, if that patch could be landed then I would abandon this one otherwise we may add a function to TargetInstrInfo.cpp if we want to use specific function.
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.
Why can't add it in unfoldMemoryOperand
if it is required in each time?
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.
If that then it's same to my solution which disable unfold for RMW, and we need to clarify if RMW would not be used every where or just here.(Although this signature for unfoldMemoryOperand just used here, I don't know if it would be used in the future.
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 don't understand this PR, either. Shouldn't #67288 be enough?
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.
Yes, I raised this first, then I find it can also be fixed by #67288. If that patch landed then this patch should be abandoned.
Closed due to #67288 landed. |
Fast scheduler added in 2008 patch and this fix in ScheduleDAGRRList.cpp started at 2011 patch. So I guess this could be missed.